Date: Sun, 21 May 2017 20:33:04 -0700 From: Kirk McKusick <mckusick@mckusick.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Jilles Tjoelker <jilles@stack.nl>, freebsd-fs@freebsd.org Subject: Re: 64-bit inodes (ino64) Status Update and Call for Testing Message-ID: <201705220333.v4M3X43q007873@chez.mckusick.com> In-Reply-To: <20170521142535.GI1622@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
I think that kib's proposed change is a sensible thing to do and
it does make sense to do it in the context of this change. I regret
that it is coming so late in the testing phase of this commit and
that is my only reluctancy in making it.
If Peter Holm (pho@) can at least run his test suite with this
change and if it does not turn up any problems, then I would be
inclined to make kib's proposed change. But I fully understand not
wanting to risk screwing up this patch with this last minute change.
Kirk McKusick
=3D-=3D-=3D
Date: Sun, 21 May 2017 17:25:35 +0300
From: Konstantin Belousov <kostikbel@gmail.com>
To: Jilles Tjoelker <jilles@stack.nl>
Subject: Re: 64-bit inodes (ino64) Status Update and Call for Testing
On Sun, May 21, 2017 at 04:03:55PM +0200, Jilles Tjoelker wrote:
> On Sun, May 21, 2017 at 03:31:18PM +0300, Konstantin Belousov wrote:
>> On Sun, May 21, 2017 at 02:14:56PM +0200, Jilles Tjoelker wrote:
>>> We have another type in this area which is too small in some situation=
s:
>>> uint8_t for struct dirent.d_namlen. For filesystems that store filenam=
es
>>> as upto 255 UTF-16 code units, the name to be stored in d_name may be
>>> upto 765 bytes long in UTF-8. This was reported in PR 204643. The code
>>> currently handles this by returning the short (8.3) name, but this nam=
e
>>> may not be present or usable, leaving the file inaccessible.
> =
>>> Actually allowing longer names seems too complicated to add to the ino=
64
>>> change, but changing d_namlen to uint16_t (using d_pad0 space) and
>>> skipping entries with d_namlen > 255 in libc may be helpful.
> =
>>> Note that applications using the deprecated readdir_r() will not be ab=
le
>>> to read such long names, since the API does not allow specifying that =
a
>>> larger buffer has been provided. (This could be avoided by making stru=
ct
>>> dirent.d_name 766 bytes long instead of 256.)
> =
>>> Unfortunately, the existence of readdir_r() also prevents changing
>>> struct dirent.d_name to the more correct flexible array.
> =
>> Yes, changing the size of d_name at this stage of the project is out of
>> question. My reading of your proposal is that we should extend the size
>> of d_namlen to uint16_t, am I right ? Should we go to 32bit directly
>> then, perhaps ?
> =
> Yes, my proposal is to change d_namlen to uint16_t.
> =
> Making it 32 bits is not useful with the 16-bit d_reclen, and increasing
> d_reclen does not seem useful to me with the current model of
> getdirentries() where the whole dirent must fit into the caller's
> buffer.
Bumping it now might cause less churn later, even if unused, but ok.
> =
>> I did not committed the change below, nor did I tested or even build it=
.
> =
> I'd like to skip overlong names in the native readdir_r() as well, so
> that long name support can be added to the kernel later without causing
> buffer overflows with applications using FreeBSD 12.0 libc.
> =
> The native readdir() does not seem to have such a problem.
Again, not even compiled.
diff --git a/lib/libc/gen/readdir-compat11.c b/lib/libc/gen/readdir-compat=
11.c
index 1c52f563c75..a865ab9157e 100644
--- a/lib/libc/gen/readdir-compat11.c
+++ b/lib/libc/gen/readdir-compat11.c
@@ -41,6 +41,7 @@ __FBSDID("$FreeBSD$");
#define _WANT_FREEBSD11_DIRENT
#include <dirent.h>
#include <errno.h>
+#include <stdbool.h>
#include <stddef.h>
#include <stdlib.h>
#include <string.h>
@@ -53,10 +54,12 @@ __FBSDID("$FreeBSD$");
#include "gen-compat.h"
-static void
+static bool
freebsd11_cvtdirent(struct freebsd11_dirent *dstdp, struct dirent *srcdp)
{
+ if (srcdp->d_namelen >=3D sizeof(dstdp->d_name))
+ return (false);
dstdp->d_type =3D srcdp->d_type;
dstdp->d_namlen =3D srcdp->d_namlen;
dstdp->d_fileno =3D srcdp->d_fileno; /* truncate */
@@ -65,6 +68,7 @@ freebsd11_cvtdirent(struct freebsd11_dirent *dstdp, stru=
ct dirent *srcdp)
bzero(dstdp->d_name + dstdp->d_namlen,
dstdp->d_reclen - offsetof(struct freebsd11_dirent, d_name) -
dstdp->d_namlen);
+ return (true);
}
struct freebsd11_dirent *
@@ -75,13 +79,15 @@ freebsd11_readdir(DIR *dirp)
if (__isthreaded)
_pthread_mutex_lock(&dirp->dd_lock);
- dp =3D _readdir_unlocked(dirp, 1);
+ dp =3D _readdir_unlocked(dirp, RDU_SKIP);
if (dp !=3D NULL) {
if (dirp->dd_compat_de =3D=3D NULL)
dirp->dd_compat_de =3D malloc(sizeof(struct
freebsd11_dirent));
- freebsd11_cvtdirent(dirp->dd_compat_de, dp);
- dstdp =3D dirp->dd_compat_de;
+ if (freebsd11_cvtdirent(dirp->dd_compat_de, dp))
+ dstdp =3D dirp->dd_compat_de;
+ else
+ dstdp =3D NULL;
} else
dstdp =3D NULL;
if (__isthreaded)
@@ -101,8 +107,10 @@ freebsd11_readdir_r(DIR *dirp, struct freebsd11_diren=
t *entry,
if (error !=3D 0)
return (error);
if (xresult !=3D NULL) {
- freebsd11_cvtdirent(entry, &xentry);
- *result =3D entry;
+ if (freebsd11_cvtdirent(entry, &xentry))
+ *result =3D entry;
+ else /* should not happen due to RDU_SHORT */
+ *result =3D NULL;
} else
*result =3D NULL;
return (0);
diff --git a/lib/libc/gen/readdir.c b/lib/libc/gen/readdir.c
index dc7b0df18b2..c30d48273b8 100644
--- a/lib/libc/gen/readdir.c
+++ b/lib/libc/gen/readdir.c
@@ -49,7 +49,7 @@ __FBSDID("$FreeBSD$");
* get next entry in a directory.
*/
struct dirent *
-_readdir_unlocked(DIR *dirp, int skip)
+_readdir_unlocked(DIR *dirp, int flags)
{
struct dirent *dp;
long initial_seek;
@@ -80,10 +80,13 @@ _readdir_unlocked(DIR *dirp, int skip)
dp->d_reclen > dirp->dd_len + 1 - dirp->dd_loc)
return (NULL);
dirp->dd_loc +=3D dp->d_reclen;
- if (dp->d_ino =3D=3D 0 && skip)
+ if (dp->d_ino =3D=3D 0 && (flags & RDU_SKIP) !=3D 0)
continue;
if (dp->d_type =3D=3D DT_WHT && (dirp->dd_flags & DTF_HIDEW))
continue;
+ if (dp->d_namlen >=3D sizeof(d_namlen) &&
+ (flags & RDU_SHORT) !=3D 0)
+ continue;
return (dp);
}
}
@@ -91,15 +94,13 @@ _readdir_unlocked(DIR *dirp, int skip)
struct dirent *
readdir(DIR *dirp)
{
- struct dirent *dp;
+ struct dirent *dp;
- if (__isthreaded) {
+ if (__isthreaded)
_pthread_mutex_lock(&dirp->dd_lock);
- dp =3D _readdir_unlocked(dirp, 1);
+ dp =3D _readdir_unlocked(dirp, RDU_SKIP);
+ if (__isthreaded)
_pthread_mutex_unlock(&dirp->dd_lock);
- }
- else
- dp =3D _readdir_unlocked(dirp, 1);
return (dp);
}
@@ -111,14 +112,13 @@ __readdir_r(DIR *dirp, struct dirent *entry, struct =
dirent **result)
saved_errno =3D errno;
errno =3D 0;
- if (__isthreaded) {
+ if (__isthreaded)
_pthread_mutex_lock(&dirp->dd_lock);
- if ((dp =3D _readdir_unlocked(dirp, 1)) !=3D NULL)
- memcpy(entry, dp, _GENERIC_DIRSIZ(dp));
- _pthread_mutex_unlock(&dirp->dd_lock);
- }
- else if ((dp =3D _readdir_unlocked(dirp, 1)) !=3D NULL)
+ dp =3D _readdir_unlocked(dirp, RDU_SKIP | RDU_SHORT);
+ if (dp !=3D NULL)
memcpy(entry, dp, _GENERIC_DIRSIZ(dp));
+ if (__isthreaded)
+ _pthread_mutex_unlock(&dirp->dd_lock);
if (errno !=3D 0) {
if (dp =3D=3D NULL)
diff --git a/lib/libc/gen/telldir.h b/lib/libc/gen/telldir.h
index 96ff669a312..50adb351e91 100644
--- a/lib/libc/gen/telldir.h
+++ b/lib/libc/gen/telldir.h
@@ -66,4 +66,7 @@ void _reclaim_telldir(DIR *);
void _seekdir(DIR *, long);
void _fixtelldir(DIR *dirp, long oldseek, long oldloc);
+#define RDU_SKIP 0x0001
+#define RDU_SHORT 0x0002
+
#endif
diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index 784af836aee..27b2635030d 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -3733,7 +3733,8 @@ freebsd11_kern_getdirentries(struct thread *td, int =
fd, char *ubuf, u_int count,
if (dp->d_reclen =3D=3D 0)
break;
MPASS(dp->d_reclen >=3D _GENERIC_DIRLEN(0));
- /* dp->d_namlen <=3D sizeof(dstdp.d_name) - 1 always */
+ if (dp->d_namlen >=3D sizeof(dstdp.d_name))
+ continue;
dstdp.d_type =3D dp->d_type;
dstdp.d_namlen =3D dp->d_namlen;
dstdp.d_fileno =3D dp->d_fileno; /* truncate */
diff --git a/sys/sys/dirent.h b/sys/sys/dirent.h
index 341855d0530..472142f960b 100644
--- a/sys/sys/dirent.h
+++ b/sys/sys/dirent.h
@@ -58,8 +58,7 @@ typedef __off_t off_t;
*
* Explicit padding between the last member of the header (d_namelen) and
* d_name avoids ABI padding at the end of dirent on LP64 architectures.
- * There is code depending on d_name being last. Also, retaining this
- * padding on ILP32 architectures simplifies the compat32 layer.
+ * There is code depending on d_name being last.
*/
struct dirent {
@@ -67,8 +66,9 @@ struct dirent {
off_t d_off; /* directory offset of entry */
__uint16_t d_reclen; /* length of this record */
__uint8_t d_type; /* file type, see below */
- __uint8_t d_namlen; /* length of string in d_name */
- __uint32_t d_pad0;
+ __uint8_t d_pad0;
+ __uint16_t d_namlen; /* length of string in d_name */
+ __uint16_t d_pad1;
#if __BSD_VISIBLE
#define MAXNAMLEN 255
char d_name[MAXNAMLEN + 1]; /* name must be no longer than this */
_______________________________________________
freebsd-fs@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-fs
To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201705220333.v4M3X43q007873>