| Message ID | 2025022644-blinked-broadness-c810@gregkh (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "libfs: Use d_children list to iterate simple_offset directories" | expand |
On 2/26/25 9:29 AM, Greg Kroah-Hartman wrote: > This reverts commit b9b588f22a0c049a14885399e27625635ae6ef91. > > There are reports of this commit breaking Chrome's rendering mode. As > no one seems to want to do a root-cause, let's just revert it for now as > it is affecting people using the latest release as well as the stable > kernels that it has been backported to. NACK. This re-introduces a CVE. The problem is we don't have a reproducer yet. > Link: https://lore.kernel.org/r/874j0lvy89.wl-tiwai@suse.de > Fixes: b9b588f22a0c ("libfs: Use d_children list to iterate simple_offset directories") > Cc: stable <stable@kernel.org> > Reported-by: Takashi Iwai <tiwai@suse.de> > Cc: Chuck Lever <chuck.lever@oracle.com> > Cc: Christian Brauner <brauner@kernel.org> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > fs/libfs.c | 90 ++++++++++++++++++------------------------------------ > 1 file changed, 29 insertions(+), 61 deletions(-) > > diff --git a/fs/libfs.c b/fs/libfs.c > index 8444f5cc4064..96f491f82f99 100644 > --- a/fs/libfs.c > +++ b/fs/libfs.c > @@ -247,13 +247,12 @@ EXPORT_SYMBOL(simple_dir_inode_operations); > > /* simple_offset_add() never assigns these to a dentry */ > enum { > - DIR_OFFSET_FIRST = 2, /* Find first real entry */ > DIR_OFFSET_EOD = S32_MAX, > }; > > /* simple_offset_add() allocation range */ > enum { > - DIR_OFFSET_MIN = DIR_OFFSET_FIRST + 1, > + DIR_OFFSET_MIN = 2, > DIR_OFFSET_MAX = DIR_OFFSET_EOD - 1, > }; > > @@ -458,82 +457,51 @@ static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence) > return vfs_setpos(file, offset, LONG_MAX); > } > > -static struct dentry *find_positive_dentry(struct dentry *parent, > - struct dentry *dentry, > - bool next) > +static struct dentry *offset_find_next(struct offset_ctx *octx, loff_t offset) > { > - struct dentry *found = NULL; > - > - spin_lock(&parent->d_lock); > - if (next) > - dentry = d_next_sibling(dentry); > - else if (!dentry) > - dentry = d_first_child(parent); > - hlist_for_each_entry_from(dentry, d_sib) { > - if (!simple_positive(dentry)) > - continue; > - spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED); > - if (simple_positive(dentry)) > - found = dget_dlock(dentry); > - spin_unlock(&dentry->d_lock); > - if (likely(found)) > - break; > - } > - spin_unlock(&parent->d_lock); > - return found; > -} > - > -static noinline_for_stack struct dentry * > -offset_dir_lookup(struct dentry *parent, loff_t offset) > -{ > - struct inode *inode = d_inode(parent); > - struct offset_ctx *octx = inode->i_op->get_offset_ctx(inode); > + MA_STATE(mas, &octx->mt, offset, offset); > struct dentry *child, *found = NULL; > > - MA_STATE(mas, &octx->mt, offset, offset); > - > - if (offset == DIR_OFFSET_FIRST) > - found = find_positive_dentry(parent, NULL, false); > - else { > - rcu_read_lock(); > - child = mas_find(&mas, DIR_OFFSET_MAX); > - found = find_positive_dentry(parent, child, false); > - rcu_read_unlock(); > - } > + rcu_read_lock(); > + child = mas_find(&mas, DIR_OFFSET_MAX); > + if (!child) > + goto out; > + spin_lock(&child->d_lock); > + if (simple_positive(child)) > + found = dget_dlock(child); > + spin_unlock(&child->d_lock); > +out: > + rcu_read_unlock(); > return found; > } > > static bool offset_dir_emit(struct dir_context *ctx, struct dentry *dentry) > { > struct inode *inode = d_inode(dentry); > + long offset = dentry2offset(dentry); > > - return dir_emit(ctx, dentry->d_name.name, dentry->d_name.len, > - inode->i_ino, fs_umode_to_dtype(inode->i_mode)); > + return ctx->actor(ctx, dentry->d_name.name, dentry->d_name.len, offset, > + inode->i_ino, fs_umode_to_dtype(inode->i_mode)); > } > > -static void offset_iterate_dir(struct file *file, struct dir_context *ctx) > +static void offset_iterate_dir(struct inode *inode, struct dir_context *ctx) > { > - struct dentry *dir = file->f_path.dentry; > + struct offset_ctx *octx = inode->i_op->get_offset_ctx(inode); > struct dentry *dentry; > > - dentry = offset_dir_lookup(dir, ctx->pos); > - if (!dentry) > - goto out_eod; > while (true) { > - struct dentry *next; > - > - ctx->pos = dentry2offset(dentry); > - if (!offset_dir_emit(ctx, dentry)) > - break; > - > - next = find_positive_dentry(dir, dentry, true); > - dput(dentry); > - > - if (!next) > + dentry = offset_find_next(octx, ctx->pos); > + if (!dentry) > goto out_eod; > - dentry = next; > + > + if (!offset_dir_emit(ctx, dentry)) { > + dput(dentry); > + break; > + } > + > + ctx->pos = dentry2offset(dentry) + 1; > + dput(dentry); > } > - dput(dentry); > return; > > out_eod: > @@ -572,7 +540,7 @@ static int offset_readdir(struct file *file, struct dir_context *ctx) > if (!dir_emit_dots(file, ctx)) > return 0; > if (ctx->pos != DIR_OFFSET_EOD) > - offset_iterate_dir(file, ctx); > + offset_iterate_dir(d_inode(dir), ctx); > return 0; > } >
On Wed, Feb 26, 2025 at 10:57:48AM -0500, Chuck Lever wrote: > On 2/26/25 9:29 AM, Greg Kroah-Hartman wrote: > > This reverts commit b9b588f22a0c049a14885399e27625635ae6ef91. > > > > There are reports of this commit breaking Chrome's rendering mode. As > > no one seems to want to do a root-cause, let's just revert it for now as > > it is affecting people using the latest release as well as the stable > > kernels that it has been backported to. > > NACK. This re-introduces a CVE. As I said elsewhere, when a commit that is assigned a CVE is reverted, then the CVE gets revoked. But I don't see this commit being assigned to a CVE, so what CVE specifically are you referring to? thanks, greg k-h
On 2/26/25 11:21 AM, Greg Kroah-Hartman wrote: > On Wed, Feb 26, 2025 at 10:57:48AM -0500, Chuck Lever wrote: >> On 2/26/25 9:29 AM, Greg Kroah-Hartman wrote: >>> This reverts commit b9b588f22a0c049a14885399e27625635ae6ef91. >>> >>> There are reports of this commit breaking Chrome's rendering mode. As >>> no one seems to want to do a root-cause, let's just revert it for now as >>> it is affecting people using the latest release as well as the stable >>> kernels that it has been backported to. >> >> NACK. This re-introduces a CVE. > > As I said elsewhere, when a commit that is assigned a CVE is reverted, > then the CVE gets revoked. But I don't see this commit being assigned > to a CVE, so what CVE specifically are you referring to? https://nvd.nist.gov/vuln/detail/CVE-2024-46701 The guideline that "regressions are more important than CVEs" is interesting. I hadn't heard that before. Still, it seems like we haven't had a chance to actually work on this issue yet. It could be corrected by a simple fix. Reverting seems premature to me.
On Wed, Feb 26, 2025 at 11:28:35AM -0500, Chuck Lever wrote: > On 2/26/25 11:21 AM, Greg Kroah-Hartman wrote: > > On Wed, Feb 26, 2025 at 10:57:48AM -0500, Chuck Lever wrote: > >> On 2/26/25 9:29 AM, Greg Kroah-Hartman wrote: > >>> This reverts commit b9b588f22a0c049a14885399e27625635ae6ef91. > >>> > >>> There are reports of this commit breaking Chrome's rendering mode. As > >>> no one seems to want to do a root-cause, let's just revert it for now as > >>> it is affecting people using the latest release as well as the stable > >>> kernels that it has been backported to. > >> > >> NACK. This re-introduces a CVE. > > > > As I said elsewhere, when a commit that is assigned a CVE is reverted, > > then the CVE gets revoked. But I don't see this commit being assigned > > to a CVE, so what CVE specifically are you referring to? > > https://nvd.nist.gov/vuln/detail/CVE-2024-46701 That refers to commit 64a7ce76fb90 ("libfs: fix infinite directory reads for offset dir"), which showed up in 6.11 (and only backported to 6.10.7 (which is long end-of-life). Commit b9b588f22a0c ("libfs: Use d_children list to iterate simple_offset directories") is in 6.14-rc1 and has been backported to 6.6.75, 6.12.12, and 6.13.1. I don't understand the interaction here, sorry. > The guideline that "regressions are more important than CVEs" is > interesting. I hadn't heard that before. CVEs should not be relevant for development given that we create 10-11 of them a day. Treat them like any other public bug list please. But again, I don't understand how reverting this commit relates to the CVE id you pointed at, what am I missing? > Still, it seems like we haven't had a chance to actually work on this > issue yet. It could be corrected by a simple fix. Reverting seems > premature to me. I'll let that be up to the vfs maintainers, but I'd push for reverting first to fix the regression and then taking the time to find the real change going forward to make our user's lives easier. Especially as I don't know who is working on that "simple fix" :) thanks, greg k-h
On 2/26/25 2:13 PM, Greg Kroah-Hartman wrote: > On Wed, Feb 26, 2025 at 11:28:35AM -0500, Chuck Lever wrote: >> On 2/26/25 11:21 AM, Greg Kroah-Hartman wrote: >>> On Wed, Feb 26, 2025 at 10:57:48AM -0500, Chuck Lever wrote: >>>> On 2/26/25 9:29 AM, Greg Kroah-Hartman wrote: >>>>> This reverts commit b9b588f22a0c049a14885399e27625635ae6ef91. >>>>> >>>>> There are reports of this commit breaking Chrome's rendering mode. As >>>>> no one seems to want to do a root-cause, let's just revert it for now as >>>>> it is affecting people using the latest release as well as the stable >>>>> kernels that it has been backported to. >>>> >>>> NACK. This re-introduces a CVE. >>> >>> As I said elsewhere, when a commit that is assigned a CVE is reverted, >>> then the CVE gets revoked. But I don't see this commit being assigned >>> to a CVE, so what CVE specifically are you referring to? >> >> https://nvd.nist.gov/vuln/detail/CVE-2024-46701 > > That refers to commit 64a7ce76fb90 ("libfs: fix infinite directory reads > for offset dir"), which showed up in 6.11 (and only backported to 6.10.7 > (which is long end-of-life). Commit b9b588f22a0c ("libfs: Use > d_children list to iterate simple_offset directories") is in 6.14-rc1 > and has been backported to 6.6.75, 6.12.12, and 6.13.1. > > I don't understand the interaction here, sorry. Commit 64a7ce76fb90 is an attempt to fix the infinite loop, but can not be applied to kernels before 0e4a862174f2 ("libfs: Convert simple directory offsets to use a Maple Tree"), even though those kernels also suffer from the looping symptoms described in the CVE. There was significant controversy (which you responded to) when Yu Kuai <yukuai3@huawei.com> attempted a backport of 64a7ce76fb90 to address this CVE in v6.6 by first applying all upstream mtree patches to v6.6. That backport was roundly rejected by Liam and Lorenzo. Commit b9b588f22a0c is a second attempt to fix the infinite loop problem that does not depend on having a working Maple tree implementation. b9b588f22a0c is a fix that can work properly with the older xarray mechanism that 0e4a862174f2 replaced, so it can be backported (with certain adjustments) to kernels before 0e4a862174f2. Note that as part of the series where b9b588f22a0c was applied, 64a7ce76fb90 is reverted (v6.10 and forward). Reverting b9b588f22a0c leaves LTS kernels from v6.6 forward with the infinite loop problem unfixed entirely because 64a7ce76fb90 has also now been reverted. >> The guideline that "regressions are more important than CVEs" is >> interesting. I hadn't heard that before. > > CVEs should not be relevant for development given that we create 10-11 > of them a day. Treat them like any other public bug list please. > > But again, I don't understand how reverting this commit relates to the > CVE id you pointed at, what am I missing? > >> Still, it seems like we haven't had a chance to actually work on this >> issue yet. It could be corrected by a simple fix. Reverting seems >> premature to me. > > I'll let that be up to the vfs maintainers, but I'd push for reverting > first to fix the regression and then taking the time to find the real > change going forward to make our user's lives easier. Especially as I > don't know who is working on that "simple fix" :) The issue is that we need the Chrome team to tell us what new system behavior is causing Chrome to malfunction. None of us have expertise to examine as complex an application as Chrome to nail the one small change that is causing the problem. This could even be a latent bug in Chrome. As soon as they have reviewed the bug and provided a simple reproducer, I will start active triage.
diff --git a/fs/libfs.c b/fs/libfs.c index 8444f5cc4064..96f491f82f99 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -247,13 +247,12 @@ EXPORT_SYMBOL(simple_dir_inode_operations); /* simple_offset_add() never assigns these to a dentry */ enum { - DIR_OFFSET_FIRST = 2, /* Find first real entry */ DIR_OFFSET_EOD = S32_MAX, }; /* simple_offset_add() allocation range */ enum { - DIR_OFFSET_MIN = DIR_OFFSET_FIRST + 1, + DIR_OFFSET_MIN = 2, DIR_OFFSET_MAX = DIR_OFFSET_EOD - 1, }; @@ -458,82 +457,51 @@ static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence) return vfs_setpos(file, offset, LONG_MAX); } -static struct dentry *find_positive_dentry(struct dentry *parent, - struct dentry *dentry, - bool next) +static struct dentry *offset_find_next(struct offset_ctx *octx, loff_t offset) { - struct dentry *found = NULL; - - spin_lock(&parent->d_lock); - if (next) - dentry = d_next_sibling(dentry); - else if (!dentry) - dentry = d_first_child(parent); - hlist_for_each_entry_from(dentry, d_sib) { - if (!simple_positive(dentry)) - continue; - spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED); - if (simple_positive(dentry)) - found = dget_dlock(dentry); - spin_unlock(&dentry->d_lock); - if (likely(found)) - break; - } - spin_unlock(&parent->d_lock); - return found; -} - -static noinline_for_stack struct dentry * -offset_dir_lookup(struct dentry *parent, loff_t offset) -{ - struct inode *inode = d_inode(parent); - struct offset_ctx *octx = inode->i_op->get_offset_ctx(inode); + MA_STATE(mas, &octx->mt, offset, offset); struct dentry *child, *found = NULL; - MA_STATE(mas, &octx->mt, offset, offset); - - if (offset == DIR_OFFSET_FIRST) - found = find_positive_dentry(parent, NULL, false); - else { - rcu_read_lock(); - child = mas_find(&mas, DIR_OFFSET_MAX); - found = find_positive_dentry(parent, child, false); - rcu_read_unlock(); - } + rcu_read_lock(); + child = mas_find(&mas, DIR_OFFSET_MAX); + if (!child) + goto out; + spin_lock(&child->d_lock); + if (simple_positive(child)) + found = dget_dlock(child); + spin_unlock(&child->d_lock); +out: + rcu_read_unlock(); return found; } static bool offset_dir_emit(struct dir_context *ctx, struct dentry *dentry) { struct inode *inode = d_inode(dentry); + long offset = dentry2offset(dentry); - return dir_emit(ctx, dentry->d_name.name, dentry->d_name.len, - inode->i_ino, fs_umode_to_dtype(inode->i_mode)); + return ctx->actor(ctx, dentry->d_name.name, dentry->d_name.len, offset, + inode->i_ino, fs_umode_to_dtype(inode->i_mode)); } -static void offset_iterate_dir(struct file *file, struct dir_context *ctx) +static void offset_iterate_dir(struct inode *inode, struct dir_context *ctx) { - struct dentry *dir = file->f_path.dentry; + struct offset_ctx *octx = inode->i_op->get_offset_ctx(inode); struct dentry *dentry; - dentry = offset_dir_lookup(dir, ctx->pos); - if (!dentry) - goto out_eod; while (true) { - struct dentry *next; - - ctx->pos = dentry2offset(dentry); - if (!offset_dir_emit(ctx, dentry)) - break; - - next = find_positive_dentry(dir, dentry, true); - dput(dentry); - - if (!next) + dentry = offset_find_next(octx, ctx->pos); + if (!dentry) goto out_eod; - dentry = next; + + if (!offset_dir_emit(ctx, dentry)) { + dput(dentry); + break; + } + + ctx->pos = dentry2offset(dentry) + 1; + dput(dentry); } - dput(dentry); return; out_eod: @@ -572,7 +540,7 @@ static int offset_readdir(struct file *file, struct dir_context *ctx) if (!dir_emit_dots(file, ctx)) return 0; if (ctx->pos != DIR_OFFSET_EOD) - offset_iterate_dir(file, ctx); + offset_iterate_dir(d_inode(dir), ctx); return 0; }
This reverts commit b9b588f22a0c049a14885399e27625635ae6ef91. There are reports of this commit breaking Chrome's rendering mode. As no one seems to want to do a root-cause, let's just revert it for now as it is affecting people using the latest release as well as the stable kernels that it has been backported to. Link: https://lore.kernel.org/r/874j0lvy89.wl-tiwai@suse.de Fixes: b9b588f22a0c ("libfs: Use d_children list to iterate simple_offset directories") Cc: stable <stable@kernel.org> Reported-by: Takashi Iwai <tiwai@suse.de> Cc: Chuck Lever <chuck.lever@oracle.com> Cc: Christian Brauner <brauner@kernel.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- fs/libfs.c | 90 ++++++++++++++++++------------------------------------ 1 file changed, 29 insertions(+), 61 deletions(-)