On Thu, 2019-07-04 at 21:18 +0000, Andreas Dilger wrote: > On Jul 4, 2019, at 04:43, Jeff Layton <[email protected]> wrote: > > On Thu, 2019-07-04 at 06:37 -0400, Jeff Layton wrote: > > > On Wed, 2019-07-03 at 20:24 +0000, Andreas Dilger wrote: > > > > When calling 'stat -c %N' to print the filename, don't explicitly > > > > request the size of the file via statx(), as it may add overhead on > > > > some filesystems. The size is only needed to optimize an allocation > > > > for the relatively rare case of reading a symlink name, and the worst > > > > effect is a somewhat-too-large temporary buffer may be allocated for > > > > areadlink_with_size(), or internal retries if buffer is too small. > > > > > > > > The file size will be returned by statx() on most filesystems, even > > > > if not requested, unless the filesystem considers this to be too > > > > expensive for that file, in which case the tradeoff is worthwhile. > > > > > > > > * src/stat.c: Don't explicitly request STATX_SIZE for filenames. > > > > Start with a 1KB buffer for areadlink_with_size() if st_size unset. > > > > --- > > > > src/stat.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/stat.c b/src/stat.c > > > > index ec0bb7d..c887013 100644 > > > > --- a/src/stat.c > > > > +++ b/src/stat.c > > > > @@ -1282,7 +1282,7 @@ fmt_to_mask (char fmt) > > > > switch (fmt) > > > > { > > > > case 'N': > > > > - return STATX_MODE|STATX_SIZE; > > > > + return STATX_MODE; > > > > case 'd': > > > > case 'D': > > > > return STATX_MODE; > > > > @@ -1491,7 +1491,9 @@ print_stat (char *pformat, size_t prefix_len, > > > > unsigned int m, > > > > out_string (pformat, prefix_len, quoteN (filename)); > > > > if (S_ISLNK (statbuf->st_mode)) > > > > { > > > > - char *linkname = areadlink_with_size (filename, > > > > statbuf->st_size); > > > > + /* if statx() didn't set size, most symlinks are under 1KB */ > > > > + char *linkname = areadlink_with_size (filename, > > > > statbuf->st_size ?: > > > > + 1023); > > > > if (linkname == NULL) > > > > { > > > > error (0, errno, _("cannot read symbolic link %s"), > > > > > > Looks reasonable to me. > > > > > > Reviewed-by: Jeff Layton <[email protected]> > > > > Actually, I'll retract that just yet... > > > > I'm not sure that statbuf->st_size will be initialized to 0 if statx > > didn't fill out the stx_size field. You may need to accompany this patch > > with one that zeroes out the stx struct in do_stat. > > The kernel will explicitly zero out struct kstat at the beginning of the > callchain > so that only valid values filled in by the filesystem are returned, in order > to > avoid leaking any kernel memory to userspace: > > int vfs_getattr_nosec(const struct path *path, struct kstat *stat, > u32 request_mask, unsigned int query_flags) > { > struct inode *inode = d_backing_inode(path->dentry); > > memset(stat, 0, sizeof(*stat)); > stat->result_mask |= STATX_BASIC_STATS; > request_mask &= STATX_ALL; > query_flags &= KSTAT_QUERY_FLAGS; > > > Both the kernel generic_fillattr() and statx_to_stat() copy all of the fields, > regardless of whether the corresponding bits are set in the request_mask or > not, > so this should be totally safe and there is no benefit to zero the struct in > userspace first.
I don't think it's wise to rely on undocumented behavior. That could easily change in the future, and what if (e.g.) BSD grows a statx implementation that doesn't do that? I'd suggest at least zeroing out the stx_size field first. -- Jeff Layton <[email protected]>
