From owner-freebsd-fs@freebsd.org Mon May 22 03:31:32 2017 Return-Path: Delivered-To: freebsd-fs@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 63739D77DFE for ; Mon, 22 May 2017 03:31:32 +0000 (UTC) (envelope-from mckusick@mckusick.com) Received: from chez.mckusick.com (chez.mckusick.com [70.36.157.235]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 47E731023 for ; Mon, 22 May 2017 03:31:31 +0000 (UTC) (envelope-from mckusick@mckusick.com) Received: from chez.mckusick.com (localhost [IPv6:::1]) by chez.mckusick.com (8.15.2/8.15.2) with ESMTP id v4M3X43q007873; Sun, 21 May 2017 20:33:04 -0700 (PDT) (envelope-from mckusick@chez.mckusick.com) Message-Id: <201705220333.v4M3X43q007873@chez.mckusick.com> From: Kirk McKusick To: Konstantin Belousov Subject: Re: 64-bit inodes (ino64) Status Update and Call for Testing cc: Jilles Tjoelker , freebsd-fs@freebsd.org In-reply-to: <20170521142535.GI1622@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <7871.1495423984.1@chez.mckusick.com> Content-Transfer-Encoding: quoted-printable Date: Sun, 21 May 2017 20:33:04 -0700 X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 22 May 2017 03:31:32 -0000 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 To: Jilles Tjoelker 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 #include +#include #include #include #include @@ -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"