| Message ID | 161017373322.1142776.5174880606166253807.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | various: random fixes | expand |
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
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
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) &&