* [PATCHSET 0/3] various: random fixes
@ 2021-01-09 6:28 Darrick J. Wong
2021-01-09 6:28 ` [PATCH 1/3] misc: fix valgrind complaints Darrick J. Wong
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Darrick J. Wong @ 2021-01-09 6:28 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
Hi all,
Here are some random fixes to the utilities: the first shuts up valgrind
warnings about uninitialized userspace memory being passed into kernel
ioctls; the second fixes warnings about libicu not being released at the
end of scrub; and the third fixes false collision reports during scrub
phase 5 if someone is replacing directory items while the scanner is
running.
If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.
This is an extraordinary way to destroy everything. Enjoy!
Comments and questions are, as always, welcome.
--D
kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes
xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes
fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
libhandle/handle.c | 7 ++++++-
scrub/inodes.c | 1 +
scrub/spacemap.c | 2 +-
scrub/unicrash.c | 33 ++++++++++++++++++++++++++++++++-
scrub/unicrash.h | 4 ++++
scrub/xfs_scrub.c | 6 ++++++
6 files changed, 50 insertions(+), 3 deletions(-)
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] misc: fix valgrind complaints 2021-01-09 6:28 [PATCHSET 0/3] various: random fixes Darrick J. Wong @ 2021-01-09 6:28 ` Darrick J. Wong 2021-01-11 13:38 ` Chandan Babu R 2021-01-11 17:27 ` Christoph Hellwig 2021-01-09 6:28 ` [PATCH 2/3] xfs_scrub: load and unload libicu properly Darrick J. Wong 2021-01-09 6:28 ` [PATCH 3/3] xfs_scrub: handle concurrent directory updates during name scan Darrick J. Wong 2 siblings, 2 replies; 12+ messages in thread From: Darrick J. Wong @ 2021-01-09 6:28 UTC (permalink / raw) To: sandeen, darrick.wong; +Cc: linux-xfs From: Darrick J. Wong <djwong@kernel.org> Zero the memory that we pass to the kernel via ioctls so that we never pass userspace heap/stack garbage around. This silences valgrind complaints about uninitialized padding areas. Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- libhandle/handle.c | 7 ++++++- scrub/inodes.c | 1 + scrub/spacemap.c | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/libhandle/handle.c b/libhandle/handle.c index 5c1686b3..a6b35b09 100644 --- a/libhandle/handle.c +++ b/libhandle/handle.c @@ -235,9 +235,12 @@ obj_to_handle( { char hbuf [MAXHANSIZ]; int ret; - uint32_t handlen; + uint32_t handlen = 0; xfs_fsop_handlereq_t hreq; + memset(&hreq, 0, sizeof(hreq)); + memset(hbuf, 0, MAXHANSIZ); + if (opcode == XFS_IOC_FD_TO_HANDLE) { hreq.fd = obj.fd; hreq.path = NULL; @@ -280,6 +283,7 @@ open_by_fshandle( if ((fsfd = handle_to_fsfd(fshanp, &path)) < 0) return -1; + memset(&hreq, 0, sizeof(hreq)); hreq.fd = 0; hreq.path = NULL; hreq.oflags = rw | O_LARGEFILE; @@ -387,6 +391,7 @@ attr_list_by_handle( if ((fd = handle_to_fsfd(hanp, &path)) < 0) return -1; + memset(&alhreq, 0, sizeof(alhreq)); alhreq.hreq.fd = 0; alhreq.hreq.path = NULL; alhreq.hreq.oflags = O_LARGEFILE; diff --git a/scrub/inodes.c b/scrub/inodes.c index 4550db83..f2bce16f 100644 --- a/scrub/inodes.c +++ b/scrub/inodes.c @@ -129,6 +129,7 @@ scan_ag_inodes( minor(ctx->fsinfo.fs_datadev), agno); + memset(&handle, 0, sizeof(handle)); memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid)); handle.ha_fid.fid_len = sizeof(xfs_fid_t) - sizeof(handle.ha_fid.fid_len); diff --git a/scrub/spacemap.c b/scrub/spacemap.c index 9653916d..9362710e 100644 --- a/scrub/spacemap.c +++ b/scrub/spacemap.c @@ -47,7 +47,7 @@ scrub_iterate_fsmap( int i; int error; - head = malloc(fsmap_sizeof(FSMAP_NR)); + head = calloc(1, fsmap_sizeof(FSMAP_NR)); if (!head) return errno; ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] misc: fix valgrind complaints 2021-01-09 6:28 ` [PATCH 1/3] misc: fix valgrind complaints Darrick J. Wong @ 2021-01-11 13:38 ` Chandan Babu R 2021-01-12 1:22 ` Darrick J. Wong 2021-01-11 17:27 ` Christoph Hellwig 1 sibling, 1 reply; 12+ messages in thread From: Chandan Babu R @ 2021-01-11 13:38 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, darrick.wong, linux-xfs On 09 Jan 2021 at 11:58, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Zero the memory that we pass to the kernel via ioctls so that we never > pass userspace heap/stack garbage around. This silences valgrind > complaints about uninitialized padding areas. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > libhandle/handle.c | 7 ++++++- > scrub/inodes.c | 1 + > scrub/spacemap.c | 2 +- > 3 files changed, 8 insertions(+), 2 deletions(-) > > > diff --git a/libhandle/handle.c b/libhandle/handle.c > index 5c1686b3..a6b35b09 100644 > --- a/libhandle/handle.c > +++ b/libhandle/handle.c > @@ -235,9 +235,12 @@ obj_to_handle( > { > char hbuf [MAXHANSIZ]; > int ret; > - uint32_t handlen; > + uint32_t handlen = 0; > xfs_fsop_handlereq_t hreq; > > + memset(&hreq, 0, sizeof(hreq)); > + memset(hbuf, 0, MAXHANSIZ); > + > if (opcode == XFS_IOC_FD_TO_HANDLE) { > hreq.fd = obj.fd; > hreq.path = NULL; > @@ -280,6 +283,7 @@ open_by_fshandle( > if ((fsfd = handle_to_fsfd(fshanp, &path)) < 0) > return -1; > > + memset(&hreq, 0, sizeof(hreq)); > hreq.fd = 0; > hreq.path = NULL; > hreq.oflags = rw | O_LARGEFILE; > @@ -387,6 +391,7 @@ attr_list_by_handle( > if ((fd = handle_to_fsfd(hanp, &path)) < 0) > return -1; > > + memset(&alhreq, 0, sizeof(alhreq)); > alhreq.hreq.fd = 0; > alhreq.hreq.path = NULL; > alhreq.hreq.oflags = O_LARGEFILE; > diff --git a/scrub/inodes.c b/scrub/inodes.c > index 4550db83..f2bce16f 100644 > --- a/scrub/inodes.c > +++ b/scrub/inodes.c > @@ -129,6 +129,7 @@ scan_ag_inodes( > minor(ctx->fsinfo.fs_datadev), > agno); > > + memset(&handle, 0, sizeof(handle)); > memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid)); > handle.ha_fid.fid_len = sizeof(xfs_fid_t) - > sizeof(handle.ha_fid.fid_len); > diff --git a/scrub/spacemap.c b/scrub/spacemap.c > index 9653916d..9362710e 100644 > --- a/scrub/spacemap.c > +++ b/scrub/spacemap.c > @@ -47,7 +47,7 @@ scrub_iterate_fsmap( > int i; > int error; > > - head = malloc(fsmap_sizeof(FSMAP_NR)); > + head = calloc(1, fsmap_sizeof(FSMAP_NR)); > if (!head) > return errno; > Minor nit: The "memset(head, 0, sizeof(*head))" statement following the above call to calloc() can now be removed. -- chandan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] misc: fix valgrind complaints 2021-01-11 13:38 ` Chandan Babu R @ 2021-01-12 1:22 ` Darrick J. Wong 0 siblings, 0 replies; 12+ messages in thread From: Darrick J. Wong @ 2021-01-12 1:22 UTC (permalink / raw) To: Chandan Babu R; +Cc: sandeen, darrick.wong, linux-xfs On Mon, Jan 11, 2021 at 07:08:27PM +0530, Chandan Babu R wrote: > > On 09 Jan 2021 at 11:58, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Zero the memory that we pass to the kernel via ioctls so that we never > > pass userspace heap/stack garbage around. This silences valgrind > > complaints about uninitialized padding areas. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > libhandle/handle.c | 7 ++++++- > > scrub/inodes.c | 1 + > > scrub/spacemap.c | 2 +- > > 3 files changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/libhandle/handle.c b/libhandle/handle.c > > index 5c1686b3..a6b35b09 100644 > > --- a/libhandle/handle.c > > +++ b/libhandle/handle.c > > @@ -235,9 +235,12 @@ obj_to_handle( > > { > > char hbuf [MAXHANSIZ]; > > int ret; > > - uint32_t handlen; > > + uint32_t handlen = 0; > > xfs_fsop_handlereq_t hreq; > > > > + memset(&hreq, 0, sizeof(hreq)); > > + memset(hbuf, 0, MAXHANSIZ); > > + > > if (opcode == XFS_IOC_FD_TO_HANDLE) { > > hreq.fd = obj.fd; > > hreq.path = NULL; > > @@ -280,6 +283,7 @@ open_by_fshandle( > > if ((fsfd = handle_to_fsfd(fshanp, &path)) < 0) > > return -1; > > > > + memset(&hreq, 0, sizeof(hreq)); > > hreq.fd = 0; > > hreq.path = NULL; > > hreq.oflags = rw | O_LARGEFILE; > > @@ -387,6 +391,7 @@ attr_list_by_handle( > > if ((fd = handle_to_fsfd(hanp, &path)) < 0) > > return -1; > > > > + memset(&alhreq, 0, sizeof(alhreq)); > > alhreq.hreq.fd = 0; > > alhreq.hreq.path = NULL; > > alhreq.hreq.oflags = O_LARGEFILE; > > diff --git a/scrub/inodes.c b/scrub/inodes.c > > index 4550db83..f2bce16f 100644 > > --- a/scrub/inodes.c > > +++ b/scrub/inodes.c > > @@ -129,6 +129,7 @@ scan_ag_inodes( > > minor(ctx->fsinfo.fs_datadev), > > agno); > > > > + memset(&handle, 0, sizeof(handle)); > > memcpy(&handle.ha_fsid, ctx->fshandle, sizeof(handle.ha_fsid)); > > handle.ha_fid.fid_len = sizeof(xfs_fid_t) - > > sizeof(handle.ha_fid.fid_len); > > diff --git a/scrub/spacemap.c b/scrub/spacemap.c > > index 9653916d..9362710e 100644 > > --- a/scrub/spacemap.c > > +++ b/scrub/spacemap.c > > @@ -47,7 +47,7 @@ scrub_iterate_fsmap( > > int i; > > int error; > > > > - head = malloc(fsmap_sizeof(FSMAP_NR)); > > + head = calloc(1, fsmap_sizeof(FSMAP_NR)); > > if (!head) > > return errno; > > > > Minor nit: The "memset(head, 0, sizeof(*head))" statement following the above > call to calloc() can now be removed. FIxed, thanks. --D > -- > chandan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] misc: fix valgrind complaints 2021-01-09 6:28 ` [PATCH 1/3] misc: fix valgrind complaints Darrick J. Wong 2021-01-11 13:38 ` Chandan Babu R @ 2021-01-11 17:27 ` Christoph Hellwig 2021-01-12 1:22 ` Darrick J. Wong 1 sibling, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2021-01-11 17:27 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, darrick.wong, linux-xfs On Fri, Jan 08, 2021 at 10:28:40PM -0800, Darrick J. Wong wrote: > char hbuf [MAXHANSIZ]; > int ret; > - uint32_t handlen; > + uint32_t handlen = 0; > xfs_fsop_handlereq_t hreq; > > + memset(&hreq, 0, sizeof(hreq)); > + memset(hbuf, 0, MAXHANSIZ); Using empty initializers at declaration time is simpler and sometimes more efficient. But either way will work fine. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] misc: fix valgrind complaints 2021-01-11 17:27 ` Christoph Hellwig @ 2021-01-12 1:22 ` Darrick J. Wong 0 siblings, 0 replies; 12+ messages in thread From: Darrick J. Wong @ 2021-01-12 1:22 UTC (permalink / raw) To: Christoph Hellwig; +Cc: sandeen, darrick.wong, linux-xfs On Mon, Jan 11, 2021 at 05:27:46PM +0000, Christoph Hellwig wrote: > On Fri, Jan 08, 2021 at 10:28:40PM -0800, Darrick J. Wong wrote: > > char hbuf [MAXHANSIZ]; > > int ret; > > - uint32_t handlen; > > + uint32_t handlen = 0; > > xfs_fsop_handlereq_t hreq; > > > > + memset(&hreq, 0, sizeof(hreq)); > > + memset(hbuf, 0, MAXHANSIZ); > > Using empty initializers at declaration time is simpler and sometimes > more efficient. But either way will work fine. I'll fix that then, and get rid of two more typedef usages. --D ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] xfs_scrub: load and unload libicu properly 2021-01-09 6:28 [PATCHSET 0/3] various: random fixes Darrick J. Wong 2021-01-09 6:28 ` [PATCH 1/3] misc: fix valgrind complaints Darrick J. Wong @ 2021-01-09 6:28 ` Darrick J. Wong 2021-01-11 14:15 ` Chandan Babu R 2021-01-09 6:28 ` [PATCH 3/3] xfs_scrub: handle concurrent directory updates during name scan Darrick J. Wong 2 siblings, 1 reply; 12+ messages in thread From: Darrick J. Wong @ 2021-01-09 6:28 UTC (permalink / raw) To: sandeen, darrick.wong; +Cc: linux-xfs From: Darrick J. Wong <djwong@kernel.org> Make sure we actually load and unload libicu properly. This isn't strictly required since the library can bootstrap itself, but unloading means fewer things for valgrind to complain about. Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- scrub/unicrash.c | 17 +++++++++++++++++ scrub/unicrash.h | 4 ++++ scrub/xfs_scrub.c | 6 ++++++ 3 files changed, 27 insertions(+) diff --git a/scrub/unicrash.c b/scrub/unicrash.c index d5d2cf20..de3217c2 100644 --- a/scrub/unicrash.c +++ b/scrub/unicrash.c @@ -722,3 +722,20 @@ unicrash_check_fs_label( return __unicrash_check_name(uc, dsc, _("filesystem label"), label, 0); } + +/* Load libicu and initialize it. */ +bool +unicrash_load(void) +{ + UErrorCode uerr = U_ZERO_ERROR; + + u_init(&uerr); + return U_FAILURE(uerr); +} + +/* Unload libicu once we're done with it. */ +void +unicrash_unload(void) +{ + u_cleanup(); +} diff --git a/scrub/unicrash.h b/scrub/unicrash.h index c3a7f385..32cae3d4 100644 --- a/scrub/unicrash.h +++ b/scrub/unicrash.h @@ -25,6 +25,8 @@ int unicrash_check_xattr_name(struct unicrash *uc, struct descr *dsc, const char *attrname); int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc, const char *label); +bool unicrash_load(void); +void unicrash_unload(void); #else # define unicrash_dir_init(u, c, b) (0) # define unicrash_xattr_init(u, c, b) (0) @@ -33,6 +35,8 @@ int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc, # define unicrash_check_dir_name(u, d, n) (0) # define unicrash_check_xattr_name(u, d, n) (0) # define unicrash_check_fs_label(u, d, n) (0) +# define unicrash_init() (0) +# define unicrash_unload() do { } while (0) #endif /* HAVE_LIBICU */ #endif /* XFS_SCRUB_UNICRASH_H_ */ diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c index 1edeb150..6b202912 100644 --- a/scrub/xfs_scrub.c +++ b/scrub/xfs_scrub.c @@ -603,6 +603,11 @@ main( setlocale(LC_ALL, ""); bindtextdomain(PACKAGE, LOCALEDIR); textdomain(PACKAGE); + if (unicrash_load()) { + fprintf(stderr, + _("%s: could initialize Unicode library.\n"), progname); + goto out; + } pthread_mutex_init(&ctx.lock, NULL); ctx.mode = SCRUB_MODE_REPAIR; @@ -788,6 +793,7 @@ main( phase_end(&all_pi, 0); if (progress_fp) fclose(progress_fp); + unicrash_unload(); /* * If we're being run as a service, the return code must fit the LSB ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] xfs_scrub: load and unload libicu properly 2021-01-09 6:28 ` [PATCH 2/3] xfs_scrub: load and unload libicu properly Darrick J. Wong @ 2021-01-11 14:15 ` Chandan Babu R 2021-01-12 1:21 ` Darrick J. Wong 0 siblings, 1 reply; 12+ messages in thread From: Chandan Babu R @ 2021-01-11 14:15 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, darrick.wong, linux-xfs On 09 Jan 2021 at 11:58, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Make sure we actually load and unload libicu properly. This isn't > strictly required since the library can bootstrap itself, but unloading > means fewer things for valgrind to complain about. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > scrub/unicrash.c | 17 +++++++++++++++++ > scrub/unicrash.h | 4 ++++ > scrub/xfs_scrub.c | 6 ++++++ > 3 files changed, 27 insertions(+) > > > diff --git a/scrub/unicrash.c b/scrub/unicrash.c > index d5d2cf20..de3217c2 100644 > --- a/scrub/unicrash.c > +++ b/scrub/unicrash.c > @@ -722,3 +722,20 @@ unicrash_check_fs_label( > return __unicrash_check_name(uc, dsc, _("filesystem label"), > label, 0); > } > + > +/* Load libicu and initialize it. */ > +bool > +unicrash_load(void) > +{ > + UErrorCode uerr = U_ZERO_ERROR; > + > + u_init(&uerr); > + return U_FAILURE(uerr); > +} > + > +/* Unload libicu once we're done with it. */ > +void > +unicrash_unload(void) > +{ > + u_cleanup(); > +} > diff --git a/scrub/unicrash.h b/scrub/unicrash.h > index c3a7f385..32cae3d4 100644 > --- a/scrub/unicrash.h > +++ b/scrub/unicrash.h > @@ -25,6 +25,8 @@ int unicrash_check_xattr_name(struct unicrash *uc, struct descr *dsc, > const char *attrname); > int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc, > const char *label); > +bool unicrash_load(void); > +void unicrash_unload(void); > #else > # define unicrash_dir_init(u, c, b) (0) > # define unicrash_xattr_init(u, c, b) (0) > @@ -33,6 +35,8 @@ int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc, > # define unicrash_check_dir_name(u, d, n) (0) > # define unicrash_check_xattr_name(u, d, n) (0) > # define unicrash_check_fs_label(u, d, n) (0) > +# define unicrash_init() (0) The above should probably be defining unicrash_load(). > +# define unicrash_unload() do { } while (0) > #endif /* HAVE_LIBICU */ > > #endif /* XFS_SCRUB_UNICRASH_H_ */ > diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c > index 1edeb150..6b202912 100644 > --- a/scrub/xfs_scrub.c > +++ b/scrub/xfs_scrub.c > @@ -603,6 +603,11 @@ main( > setlocale(LC_ALL, ""); > bindtextdomain(PACKAGE, LOCALEDIR); > textdomain(PACKAGE); > + if (unicrash_load()) { > + fprintf(stderr, > + _("%s: could initialize Unicode library.\n"), progname); > + goto out; > + } > > pthread_mutex_init(&ctx.lock, NULL); > ctx.mode = SCRUB_MODE_REPAIR; > @@ -788,6 +793,7 @@ main( > phase_end(&all_pi, 0); > if (progress_fp) > fclose(progress_fp); > + unicrash_unload(); > > /* > * If we're being run as a service, the return code must fit the LSB -- chandan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] xfs_scrub: load and unload libicu properly 2021-01-11 14:15 ` Chandan Babu R @ 2021-01-12 1:21 ` Darrick J. Wong 0 siblings, 0 replies; 12+ messages in thread From: Darrick J. Wong @ 2021-01-12 1:21 UTC (permalink / raw) To: Chandan Babu R; +Cc: sandeen, darrick.wong, linux-xfs On Mon, Jan 11, 2021 at 07:45:15PM +0530, Chandan Babu R wrote: > > On 09 Jan 2021 at 11:58, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Make sure we actually load and unload libicu properly. This isn't > > strictly required since the library can bootstrap itself, but unloading > > means fewer things for valgrind to complain about. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > scrub/unicrash.c | 17 +++++++++++++++++ > > scrub/unicrash.h | 4 ++++ > > scrub/xfs_scrub.c | 6 ++++++ > > 3 files changed, 27 insertions(+) > > > > > > diff --git a/scrub/unicrash.c b/scrub/unicrash.c > > index d5d2cf20..de3217c2 100644 > > --- a/scrub/unicrash.c > > +++ b/scrub/unicrash.c > > @@ -722,3 +722,20 @@ unicrash_check_fs_label( > > return __unicrash_check_name(uc, dsc, _("filesystem label"), > > label, 0); > > } > > + > > +/* Load libicu and initialize it. */ > > +bool > > +unicrash_load(void) > > +{ > > + UErrorCode uerr = U_ZERO_ERROR; > > + > > + u_init(&uerr); > > + return U_FAILURE(uerr); > > +} > > + > > +/* Unload libicu once we're done with it. */ > > +void > > +unicrash_unload(void) > > +{ > > + u_cleanup(); > > +} > > diff --git a/scrub/unicrash.h b/scrub/unicrash.h > > index c3a7f385..32cae3d4 100644 > > --- a/scrub/unicrash.h > > +++ b/scrub/unicrash.h > > @@ -25,6 +25,8 @@ int unicrash_check_xattr_name(struct unicrash *uc, struct descr *dsc, > > const char *attrname); > > int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc, > > const char *label); > > +bool unicrash_load(void); > > +void unicrash_unload(void); > > #else > > # define unicrash_dir_init(u, c, b) (0) > > # define unicrash_xattr_init(u, c, b) (0) > > @@ -33,6 +35,8 @@ int unicrash_check_fs_label(struct unicrash *uc, struct descr *dsc, > > # define unicrash_check_dir_name(u, d, n) (0) > > # define unicrash_check_xattr_name(u, d, n) (0) > > # define unicrash_check_fs_label(u, d, n) (0) > > +# define unicrash_init() (0) > > The above should probably be defining unicrash_load(). Yep, thanks for catching that. --D > > +# define unicrash_unload() do { } while (0) > > #endif /* HAVE_LIBICU */ > > > > #endif /* XFS_SCRUB_UNICRASH_H_ */ > > diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c > > index 1edeb150..6b202912 100644 > > --- a/scrub/xfs_scrub.c > > +++ b/scrub/xfs_scrub.c > > @@ -603,6 +603,11 @@ main( > > setlocale(LC_ALL, ""); > > bindtextdomain(PACKAGE, LOCALEDIR); > > textdomain(PACKAGE); > > + if (unicrash_load()) { > > + fprintf(stderr, > > + _("%s: could initialize Unicode library.\n"), progname); > > + goto out; > > + } > > > > pthread_mutex_init(&ctx.lock, NULL); > > ctx.mode = SCRUB_MODE_REPAIR; > > @@ -788,6 +793,7 @@ main( > > phase_end(&all_pi, 0); > > if (progress_fp) > > fclose(progress_fp); > > + unicrash_unload(); > > > > /* > > * If we're being run as a service, the return code must fit the LSB > > > -- > chandan ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] xfs_scrub: handle concurrent directory updates during name scan 2021-01-09 6:28 [PATCHSET 0/3] various: random fixes Darrick J. Wong 2021-01-09 6:28 ` [PATCH 1/3] misc: fix valgrind complaints Darrick J. Wong 2021-01-09 6:28 ` [PATCH 2/3] xfs_scrub: load and unload libicu properly Darrick J. Wong @ 2021-01-09 6:28 ` Darrick J. Wong 2021-01-12 11:15 ` Chandan Babu R 2 siblings, 1 reply; 12+ messages in thread From: Darrick J. Wong @ 2021-01-09 6:28 UTC (permalink / raw) To: sandeen, darrick.wong; +Cc: linux-xfs From: Darrick J. Wong <djwong@kernel.org> The name scanner in xfs_scrub cannot lock a namespace (dirent or xattr) and the kernel does not provide a stable cursor interface, which means that we can see the same byte sequence multiple times during a scan. This isn't a confusing name error since the kernel enforces uniqueness on the byte sequence, so all we need to do here is update the old entry. Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- scrub/unicrash.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/scrub/unicrash.c b/scrub/unicrash.c index de3217c2..f5407b5e 100644 --- a/scrub/unicrash.c +++ b/scrub/unicrash.c @@ -68,7 +68,7 @@ struct name_entry { xfs_ino_t ino; - /* Raw UTF8 name */ + /* Raw dirent name */ size_t namelen; char name[0]; }; @@ -627,6 +627,20 @@ unicrash_add( uc->buckets[bucket] = new_entry; while (entry != NULL) { + /* + * If we see the same byte sequence then someone's modifying + * the namespace while we're scanning it. Update the existing + * entry's inode mapping and erase the new entry from existence. + */ + if (new_entry->namelen == entry->namelen && + !memcmp(new_entry->name, entry->name, entry->namelen)) { + entry->ino = new_entry->ino; + uc->buckets[bucket] = new_entry->next; + name_entry_free(new_entry); + *badflags = 0; + continue; + } + /* Same normalization? */ if (new_entry->normstrlen == entry->normstrlen && !u_strcmp(new_entry->normstr, entry->normstr) && ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] xfs_scrub: handle concurrent directory updates during name scan 2021-01-09 6:28 ` [PATCH 3/3] xfs_scrub: handle concurrent directory updates during name scan Darrick J. Wong @ 2021-01-12 11:15 ` Chandan Babu R 2021-01-12 17:13 ` Darrick J. Wong 0 siblings, 1 reply; 12+ messages in thread From: Chandan Babu R @ 2021-01-12 11:15 UTC (permalink / raw) To: Darrick J. Wong; +Cc: sandeen, darrick.wong, linux-xfs On 09 Jan 2021 at 11:58, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > The name scanner in xfs_scrub cannot lock a namespace (dirent or xattr) > and the kernel does not provide a stable cursor interface, which means > that we can see the same byte sequence multiple times during a scan. > This isn't a confusing name error since the kernel enforces uniqueness > on the byte sequence, so all we need to do here is update the old entry. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > scrub/unicrash.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > > diff --git a/scrub/unicrash.c b/scrub/unicrash.c > index de3217c2..f5407b5e 100644 > --- a/scrub/unicrash.c > +++ b/scrub/unicrash.c > @@ -68,7 +68,7 @@ struct name_entry { > > xfs_ino_t ino; > > - /* Raw UTF8 name */ > + /* Raw dirent name */ > size_t namelen; > char name[0]; > }; > @@ -627,6 +627,20 @@ unicrash_add( > uc->buckets[bucket] = new_entry; > > while (entry != NULL) { > + /* > + * If we see the same byte sequence then someone's modifying > + * the namespace while we're scanning it. Update the existing > + * entry's inode mapping and erase the new entry from existence. > + */ > + if (new_entry->namelen == entry->namelen && > + !memcmp(new_entry->name, entry->name, entry->namelen)) { > + entry->ino = new_entry->ino; > + uc->buckets[bucket] = new_entry->next; > + name_entry_free(new_entry); > + *badflags = 0; > + continue; If the above condition evaluates to true, the memory pointed to by "new_entry" is freed. The "continue" statement would cause the while loop to be executed once more. At this stage, "entry" will still have the previously held non-NULL value and hence the while loop is executed once more causing the invalid address in "new_entry" to be dereferenced. > + } > + > /* Same normalization? */ > if (new_entry->normstrlen == entry->normstrlen && > !u_strcmp(new_entry->normstr, entry->normstr) && -- chandan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] xfs_scrub: handle concurrent directory updates during name scan 2021-01-12 11:15 ` Chandan Babu R @ 2021-01-12 17:13 ` Darrick J. Wong 0 siblings, 0 replies; 12+ messages in thread From: Darrick J. Wong @ 2021-01-12 17:13 UTC (permalink / raw) To: Chandan Babu R; +Cc: sandeen, darrick.wong, linux-xfs On Tue, Jan 12, 2021 at 04:45:35PM +0530, Chandan Babu R wrote: > > On 09 Jan 2021 at 11:58, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > The name scanner in xfs_scrub cannot lock a namespace (dirent or xattr) > > and the kernel does not provide a stable cursor interface, which means > > that we can see the same byte sequence multiple times during a scan. > > This isn't a confusing name error since the kernel enforces uniqueness > > on the byte sequence, so all we need to do here is update the old entry. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > scrub/unicrash.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/scrub/unicrash.c b/scrub/unicrash.c > > index de3217c2..f5407b5e 100644 > > --- a/scrub/unicrash.c > > +++ b/scrub/unicrash.c > > @@ -68,7 +68,7 @@ struct name_entry { > > > > xfs_ino_t ino; > > > > - /* Raw UTF8 name */ > > + /* Raw dirent name */ > > size_t namelen; > > char name[0]; > > }; > > @@ -627,6 +627,20 @@ unicrash_add( > > uc->buckets[bucket] = new_entry; > > > > while (entry != NULL) { > > + /* > > + * If we see the same byte sequence then someone's modifying > > + * the namespace while we're scanning it. Update the existing > > + * entry's inode mapping and erase the new entry from existence. > > + */ > > + if (new_entry->namelen == entry->namelen && > > + !memcmp(new_entry->name, entry->name, entry->namelen)) { > > + entry->ino = new_entry->ino; > > + uc->buckets[bucket] = new_entry->next; > > + name_entry_free(new_entry); > > + *badflags = 0; > > + continue; > > If the above condition evaluates to true, the memory pointed to by "new_entry" > is freed. The "continue" statement would cause the while loop to be executed > once more. At this stage, "entry" will still have the previously held non-NULL > value and hence the while loop is executed once more causing the invalid > address in "new_entry" to be dereferenced. Oops, good catch! Will fix. --D > > + } > > + > > /* Same normalization? */ > > if (new_entry->normstrlen == entry->normstrlen && > > !u_strcmp(new_entry->normstr, entry->normstr) && > > > -- > chandan ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-01-12 17:14 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-09 6:28 [PATCHSET 0/3] various: random fixes Darrick J. Wong 2021-01-09 6:28 ` [PATCH 1/3] misc: fix valgrind complaints Darrick J. Wong 2021-01-11 13:38 ` Chandan Babu R 2021-01-12 1:22 ` Darrick J. Wong 2021-01-11 17:27 ` Christoph Hellwig 2021-01-12 1:22 ` Darrick J. Wong 2021-01-09 6:28 ` [PATCH 2/3] xfs_scrub: load and unload libicu properly Darrick J. Wong 2021-01-11 14:15 ` Chandan Babu R 2021-01-12 1:21 ` Darrick J. Wong 2021-01-09 6:28 ` [PATCH 3/3] xfs_scrub: handle concurrent directory updates during name scan Darrick J. Wong 2021-01-12 11:15 ` Chandan Babu R 2021-01-12 17:13 ` Darrick J. Wong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.