| Message ID | 20231025-formfrage-watscheln-84526cd3bd7d@brauner (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | file, i915: fix file reference for mmap_singleton() | expand |
On Wed, Oct 25, 2023 at 2:01 PM Christian Brauner <brauner@kernel.org> wrote: > Today we got a report at [1] for rcu stalls on the i915 testsuite in [2] > due to the conversion of files to SLAB_TYPSSAFE_BY_RCU. Afaict, > get_file_rcu() goes into an infinite loop trying to carefully verify > that i915->gem.mmap_singleton hasn't changed - see the splat below. > > So I stared at this code to figure out what it actually does. It seems > that the i915->gem.mmap_singleton pointer itself never had rcu semantics. > > The i915->gem.mmap_singleton is replaced in > file->f_op->release::singleton_release(): > > static int singleton_release(struct inode *inode, struct file *file) > { > struct drm_i915_private *i915 = file->private_data; > > cmpxchg(&i915->gem.mmap_singleton, file, NULL); > drm_dev_put(&i915->drm); > > return 0; > } > > The cmpxchg() is ordered against a concurrent update of > i915->gem.mmap_singleton from mmap_singleton(). IOW, when > mmap_singleton() fails to get a reference on i915->gem.mmap_singleton > via mmap_singleton(): > > rcu_read_lock(); > file = get_file_rcu(&i915->gem.mmap_singleton); > rcu_read_unlock(); > > mmap_singleton() allocates a new file via anon_inode_getfile() and does > > smp_store_mb(i915->gem.mmap_singleton, file); > > So, afaiu, then what happens in the case of this bug is that at some > point fput() is called and drops the file->f_count to zero but obviously > leaving the pointer in i915->gem.mmap_singleton in tact until > file->f_op->release::singleton_release() is called. > > Now, there might be possibly larger delays until > file->f_op->release::singleton_release() is called and > i915->gem.mmap_singleton is set to NULL? > > Say concurrently another task hits mmap_singleton() and does: > > rcu_read_lock(); > file = get_file_rcu(&i915->gem.mmap_singleton); > rcu_read_unlock(); > > When get_file_rcu() fails to get a reference via atomic_inc_not_zero() > it will try the reload from i915->gem.mmap_singleton assuming it has > comparable semantics as __fget_files_rcu() that also reloads on > atomic_inc_not_zero() failure. > > But since i915->gem.mmap_singleton doesn't have proper rcu semantics it > reloads the same pointer again, trying the same atomic_inc_not_zero() > again and doing so until file->f_op->release::singleton_release() of the > old file has been called. > > So, in contrast to __fget_files_rcu() here we want to not retry when > atomic_inc_not_zero() has failed. We only want to retry in case we > managed to get a reference but the pointer did change on reload. [...] > Link: [1]: https://lore.kernel.org/intel-gfx/SJ1PR11MB6129CB39EED831784C331BAFB9DEA@SJ1PR11MB6129.namprd11.prod.outlook.com > Link: [2]: https://intel-gfx-ci.01.org/tree/linux-next/next-20231013/bat-dg2-11/igt@i915_selftest@live@mman.html#dmesg-warnings10963 > Signed-off-by: Christian Brauner <brauner@kernel.org> Patch makes sense to me. I'm not sure why you changed EAGAIN to EINVAL, and it's obviously a bit ugly, but it looks like a valid fix for what the SLUB_TYPESAFE_BY_RCU conversion broke. Reviewed-by: Jann Horn <jannh@google.com> But as a side note, I am a bit confused about how the concurrency of the existing code is supposed to work... it looks like two parallel calls to mmap_singleton() can end up returning different pointers, and then the singleton is not actually a singleton anymore? If that part of the existing code is wrong even before the SLAB_TYPESAFE_BY_RCU conversion, we might later have to open-code get_file_rcu() in mmap_singleton(), so that we can do a cmpxchg at the end that checks whether the i915->gem.mmap_singleton pointer is still the same? Like: static struct file *mmap_singleton(struct drm_i915_private *i915) { struct file *file, *new_file; rcu_read_lock(); file = READ_ONCE(i915->gem.mmap_singleton); if (file && atomic_long_inc_not_zero(&file->f_count)) { rcu_read_unlock(); return file; } rcu_read_unlock(); new_file = anon_inode_getfile("i915.gem", &singleton_fops, i915, O_RDWR); if (IS_ERR(new_file)) return new_file; /* Everyone shares a single global address space */ new_file->f_mapping = i915->drm.anon_inode->i_mapping; if (try_cmpxchg(&i915->gem.mmap_singleton, &file, new_file)) { // something with drm_dev refcount ? return new_file; } // something with drm_dev refcount ? fput(new_file); return file; } It would be nice to get some i915 maintainer's comment on how the singleton stuff is supposed to work. > --- > > Jann, Linus, > > Since you've been quite involved, can you check that what I'm babbling > here makes sense? If this isn't the fix then I would have to drop the > SLAB_TYPESAFE_BY_RCU conversion patch for now since I'd like to send PRs > by the end of the week. > > This is on top of > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.misc > > Appreciate the help, thanks! > Christian > > --- > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- > fs/file.c | 17 ++++++++++++++++- > include/linux/fs.h | 1 + > 3 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > index 97bf10861cad..da97e61b18d4 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > @@ -916,7 +916,7 @@ static struct file *mmap_singleton(struct drm_i915_private *i915) > struct file *file; > > rcu_read_lock(); > - file = get_file_rcu(&i915->gem.mmap_singleton); > + file = get_file_rcu_once(&i915->gem.mmap_singleton); > rcu_read_unlock(); > if (file) > return file; > diff --git a/fs/file.c b/fs/file.c > index 1a475d7d636e..2c64d6836f0c 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -867,7 +867,7 @@ static struct file *__get_file_rcu(struct file __rcu **f) > return NULL; > > if (unlikely(!atomic_long_inc_not_zero(&file->f_count))) > - return ERR_PTR(-EAGAIN); > + return ERR_PTR(-EINVAL); > > file_reloaded = rcu_dereference_raw(*f); > > @@ -927,6 +927,21 @@ struct file *get_file_rcu(struct file __rcu **f) > } > EXPORT_SYMBOL_GPL(get_file_rcu); > > +struct file *get_file_rcu_once(struct file __rcu **f) > +{ > + for (;;) { > + struct file __rcu *file; > + > + file = __get_file_rcu(f); > + if (!IS_ERR(file)) > + return file; > + > + if (PTR_ERR(file) == -EINVAL) > + return NULL; > + } > +} > +EXPORT_SYMBOL_GPL(get_file_rcu_once); > + > static inline struct file *__fget_files_rcu(struct files_struct *files, > unsigned int fd, fmode_t mask) > { > diff --git a/include/linux/fs.h b/include/linux/fs.h > index cb8bfa1f8ecb..657bbd824490 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1044,6 +1044,7 @@ static inline struct file *get_file(struct file *f) > } > > struct file *get_file_rcu(struct file __rcu **f); > +struct file *get_file_rcu_once(struct file __rcu **f); > > #define file_count(x) atomic_long_read(&(x)->f_count) > > -- > 2.34.1 >
On Wed, 25 Oct 2023 at 02:01, Christian Brauner <brauner@kernel.org> wrote: > > rcu_read_lock(); > - file = get_file_rcu(&i915->gem.mmap_singleton); > + file = get_file_rcu_once(&i915->gem.mmap_singleton); > rcu_read_unlock(); > if (file) > return file; Honestly, the get_file_rcu_once() function seems a bit pointless. The above doesn't want a loop at all. Yet your "once" still does loop, because "even get_file_rcu_once() is trying to deal with the whole "the pointer itself changed". And the i915 code is actually designed to just depend on the atomicity of the mmap_singleton access itself, in how it uses cmpxchg(&i915->gem.mmap_singleton, file, NULL); ... file = READ_ONCE(i915->gem.mmap_singleton); and literally says "I'll remove my singleton as it is released". The important part there is that the 'map_singleton' pointer itself isn't actually reference-counted - it's the reverse, where reference-counting *other* instances will then auto-clear it. And that's what then makes that get_file_rcu() model not work with it, because get_file_rcu() kind of assumes that the argument it gets is *part* of the reference counting, not a cached *result* of the reference counting that gets cleared as a result of the ref going down to zero. I may explain my objections badly, but hopefully you get what I mean. And I think that also means that using that new get_file_rcu_once() may be hiding the actual problem, but it's still conceptually wrong, because it still has that conceptual model of "the pointer I'm getting is part of the reference counting", when it really isn't. So I think we'd actually be better off with something that is more clearly different from get_file_rcu() in naming, so make that conceptual difference clearer. Make it be something like "get_active_file(struct file **)", and make the implementation be just exactly what your current __get_file_rcu() is with no loops at all. Then thew i915 code ends up being rcu_read_lock(); file = get_active_file(&i915->gem.mmap_singleton); rcu_read_unlock(); if (!IS_ERR_OR_NULL(file)) return file; .. create new mmap_singleton .. and that's it. If you don't want to expose __get_file_rcu() as-is, you could maybe just do struct file *get_active_file(struct file **fp) { struct file *file; rcu_read_lock(); file = __get_file_rcu(fp); rcu_read_unlock(); return file; } and then the i916 code would just drop the RCU locking that it has no business even knowing about. I realize that my complaints are a bit conceptual, and that the practical end result is pretty much the same, but I do think that it is worth noting this conceptual difference between "file pointer is ref-counted" and "file counter is *controlled* by ref-counting". Add a comment to the effect at get_active_file() users. The old i915 code is already racy, in that it's called a "singleton", but if you have multiple concurrent callers to mmap_singleton(), they all might see a NULL file at first, and then they all create *separate* new "singleton" files, and they *all* do that smp_store_mb(i915->gem.mmap_singleton, file); and one random case of them happens to win the race and set *its* file as "THE singleton" file. So your "let's loop if it changes" is not fixing anything as-is, and it's just actually hiding what is going on. If the i915 code wants to be consistent and really have just one singleton, it needs to do the looping with some cmpxchg or whatever itself. Doing the loop in some get_file_rcu_once() function for when the file pointer changed isn't going to fix the race. Am I missing something? Linus
On Wed, Oct 25, 2023 at 08:52:57AM -1000, Linus Torvalds wrote: > On Wed, 25 Oct 2023 at 02:01, Christian Brauner <brauner@kernel.org> wrote: > > > > rcu_read_lock(); > > - file = get_file_rcu(&i915->gem.mmap_singleton); > > + file = get_file_rcu_once(&i915->gem.mmap_singleton); > > rcu_read_unlock(); > > if (file) > > return file; > > Honestly, the get_file_rcu_once() function seems a bit pointless. > > The above doesn't want a loop at all. Yet your "once" still does loop, > because "even get_file_rcu_once() is trying to deal with the whole > "the pointer itself changed". > > And the i915 code is actually designed to just depend on the atomicity > of the mmap_singleton access itself, in how it uses > > cmpxchg(&i915->gem.mmap_singleton, file, NULL); > ... > file = READ_ONCE(i915->gem.mmap_singleton); > > and literally says "I'll remove my singleton as it is released". The > important part there is that the 'map_singleton' pointer itself isn't > actually reference-counted - it's the reverse, where > reference-counting *other* instances will then auto-clear it. > > And that's what then makes that get_file_rcu() model not work with it, > because get_file_rcu() kind of assumes that the argument it gets is > *part* of the reference counting, not a cached *result* of the > reference counting that gets cleared as a result of the ref going down > to zero. > > I may explain my objections badly, but hopefully you get what I mean. No, I get it. I was on the fence how to deal with this because I honestly find this model here extremely weird and very unintuitive to begin with with the pointer being NULLed during release and replaced that way. > > And I think that also means that using that new get_file_rcu_once() > may be hiding the actual problem, but it's still conceptually wrong, > because it still has that conceptual model of "the pointer I'm getting > is part of the reference counting", when it really isn't. > > So I think we'd actually be better off with something that is more > clearly different from get_file_rcu() in naming, so make that > conceptual difference clearer. Make it be something like > "get_active_file(struct file **)", and make the implementation be just > exactly what your current __get_file_rcu() is with no loops at all. > > Then thew i915 code ends up being > > rcu_read_lock(); > file = get_active_file(&i915->gem.mmap_singleton); > rcu_read_unlock(); > > if (!IS_ERR_OR_NULL(file)) > return file; > > .. create new mmap_singleton .. > > and that's it. > > If you don't want to expose __get_file_rcu() as-is, you could maybe just do > > struct file *get_active_file(struct file **fp) > { > struct file *file; > rcu_read_lock(); > file = __get_file_rcu(fp); > rcu_read_unlock(); > return file; > } > > and then the i916 code would just drop the RCU locking that it has no > business even knowing about. Yeah, fair idea. I just want this fixed so we don't have to drop. Pushed to vfs.misc with your suggested changes for get_file_active() with rcu hidden in that function. Double-check and yell if something looks wrong: > The old i915 code is already racy, in that it's called a "singleton", > but if you have multiple concurrent callers to mmap_singleton(), they > all might see a NULL file at first, and then they all create > *separate* new "singleton" files, and they *all* do that > > smp_store_mb(i915->gem.mmap_singleton, file); > > and one random case of them happens to win the race and set *its* file > as "THE singleton" file. Yeah, I think that's all really weird but whatever. > Am I missing something? No, I don't think so. I thought you might have a good opinion here. I did think that the loop didn't really matter for this case but since it seemingly does paper over the weird semantics here let's drop it. Anyway, pushed to: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.misc and appended here. Please yell, if something's still off.
On Wed, Oct 25, 2023 at 08:16:23PM -0000, Patchwork wrote: > == Series Details == > > Series: file, i915: fix file reference for mmap_singleton() > URL : https://patchwork.freedesktop.org/series/125570/ > State : failure > > == Summary == > > Error: patch https://patchwork.freedesktop.org/api/1.0/series/125570/revisions/1/mbox/ not applied > Applying: file, i915: fix file reference for mmap_singleton() > Using index info to reconstruct a base tree... > M drivers/gpu/drm/i915/gem/i915_gem_mman.c > M fs/file.c > M include/linux/fs.h > Falling back to patching base and 3-way merge... > Auto-merging include/linux/fs.h > CONFLICT (content): Merge conflict in include/linux/fs.h > Auto-merging fs/file.c > CONFLICT (content): Merge conflict in fs/file.c > Auto-merging drivers/gpu/drm/i915/gem/i915_gem_mman.c > CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gem/i915_gem_mman.c > error: Failed to merge in the changes. > hint: Use 'git am --show-current-patch=diff' to see the failed patch > Patch failed at 0001 file, i915: fix file reference for mmap_singleton() > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > Build failed, no error log produced I'm not sure what tree you're testing on but please test on whatever is in vfs.misc, I guess which has that patch. git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
On Wed, 25 Oct 2023 at 10:36, Christian Brauner <brauner@kernel.org> wrote: > > I did think that the loop didn't really matter for this case but since > it seemingly does paper over the weird semantics here let's drop it. > Anyway, pushed to: > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.misc LGTM. We could then make the i915 mmap_singleton() do the proper loop over the whole thing. Not quite the way Jann suggested - which would not increment the file refcount properly, but instead of the current smp_store_mb(i915->gem.mmap_singleton, file); drm_dev_get(&i915->drm); return file; it could do something much more complicated like drm_dev_get(&i915->drm); if (cmpxchg(&i915->gem.mmap_singleton, NULL, file) == NULL) return file; // mmap_singleton wasn't NULL: it might be an old one in the // process of being torn down (with a zero refcount), or a new // one that was just installed that we should use instead fput(file); file = READ_ONCE(i915->gem.mmap_singleton); if (!file) goto repeat; // Is it valid? Just try again if (atomic_read(&file->f_count)) goto repeat; // We have a file pointer, but it's in the process of being torn // down but gem.mmap_singleton hasn't been cleared yet. Yield to // make progress. sched_yield(); goto repeat; which is disgusting, but would probably work. Note the "probably work". I'm handwaving: "something like the above". Presumably not even worth doing - I assume a correct client always just does a single mmap() before starting work. Linus
Hi, CI is always testing against https://cgit.freedesktop.org/drm-tip Br Jani > -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian > Brauner > Sent: Wednesday, October 25, 2023 11:39 PM > To: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] ✗ Fi.CI.BUILD: failure for file, i915: fix file reference for > mmap_singleton()y > > On Wed, Oct 25, 2023 at 08:16:23PM -0000, Patchwork wrote: > > == Series Details == > > > > Series: file, i915: fix file reference for mmap_singleton() > > URL : https://patchwork.freedesktop.org/series/125570/ > > State : failure > > > > == Summary == > > > > Error: patch > > https://patchwork.freedesktop.org/api/1.0/series/125570/revisions/1/mb > > ox/ not applied > > Applying: file, i915: fix file reference for mmap_singleton() Using > > index info to reconstruct a base tree... > > M drivers/gpu/drm/i915/gem/i915_gem_mman.c > > M fs/file.c > > M include/linux/fs.h > > Falling back to patching base and 3-way merge... > > Auto-merging include/linux/fs.h > > CONFLICT (content): Merge conflict in include/linux/fs.h Auto-merging > > fs/file.c CONFLICT (content): Merge conflict in fs/file.c Auto-merging > > drivers/gpu/drm/i915/gem/i915_gem_mman.c > > CONFLICT (content): Merge conflict in > > drivers/gpu/drm/i915/gem/i915_gem_mman.c > > error: Failed to merge in the changes. > > hint: Use 'git am --show-current-patch=diff' to see the failed patch > > Patch failed at 0001 file, i915: fix file reference for > > mmap_singleton() When you have resolved this problem, run "git am -- > continue". > > If you prefer to skip this patch, run "git am --skip" instead. > > To restore the original branch and stop patching, run "git am --abort". > > Build failed, no error log produced > > I'm not sure what tree you're testing on but please test on whatever is in > vfs.misc, I guess which has that patch. > git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
On 25/10/2023 16:23, Jann Horn wrote: > On Wed, Oct 25, 2023 at 2:01 PM Christian Brauner <brauner@kernel.org> wrote: >> Today we got a report at [1] for rcu stalls on the i915 testsuite in [2] >> due to the conversion of files to SLAB_TYPSSAFE_BY_RCU. Afaict, >> get_file_rcu() goes into an infinite loop trying to carefully verify >> that i915->gem.mmap_singleton hasn't changed - see the splat below. >> >> So I stared at this code to figure out what it actually does. It seems >> that the i915->gem.mmap_singleton pointer itself never had rcu semantics. >> >> The i915->gem.mmap_singleton is replaced in >> file->f_op->release::singleton_release(): >> >> static int singleton_release(struct inode *inode, struct file *file) >> { >> struct drm_i915_private *i915 = file->private_data; >> >> cmpxchg(&i915->gem.mmap_singleton, file, NULL); >> drm_dev_put(&i915->drm); >> >> return 0; >> } >> >> The cmpxchg() is ordered against a concurrent update of >> i915->gem.mmap_singleton from mmap_singleton(). IOW, when >> mmap_singleton() fails to get a reference on i915->gem.mmap_singleton >> via mmap_singleton(): >> >> rcu_read_lock(); >> file = get_file_rcu(&i915->gem.mmap_singleton); >> rcu_read_unlock(); >> >> mmap_singleton() allocates a new file via anon_inode_getfile() and does >> >> smp_store_mb(i915->gem.mmap_singleton, file); >> >> So, afaiu, then what happens in the case of this bug is that at some >> point fput() is called and drops the file->f_count to zero but obviously >> leaving the pointer in i915->gem.mmap_singleton in tact until >> file->f_op->release::singleton_release() is called. >> >> Now, there might be possibly larger delays until >> file->f_op->release::singleton_release() is called and >> i915->gem.mmap_singleton is set to NULL? >> >> Say concurrently another task hits mmap_singleton() and does: >> >> rcu_read_lock(); >> file = get_file_rcu(&i915->gem.mmap_singleton); >> rcu_read_unlock(); >> >> When get_file_rcu() fails to get a reference via atomic_inc_not_zero() >> it will try the reload from i915->gem.mmap_singleton assuming it has >> comparable semantics as __fget_files_rcu() that also reloads on >> atomic_inc_not_zero() failure. >> >> But since i915->gem.mmap_singleton doesn't have proper rcu semantics it >> reloads the same pointer again, trying the same atomic_inc_not_zero() >> again and doing so until file->f_op->release::singleton_release() of the >> old file has been called. >> >> So, in contrast to __fget_files_rcu() here we want to not retry when >> atomic_inc_not_zero() has failed. We only want to retry in case we >> managed to get a reference but the pointer did change on reload. > [...] >> Link: [1]: https://lore.kernel.org/intel-gfx/SJ1PR11MB6129CB39EED831784C331BAFB9DEA@SJ1PR11MB6129.namprd11.prod.outlook.com >> Link: [2]: https://intel-gfx-ci.01.org/tree/linux-next/next-20231013/bat-dg2-11/igt@i915_selftest@live@mman.html#dmesg-warnings10963 >> Signed-off-by: Christian Brauner <brauner@kernel.org> > > Patch makes sense to me. I'm not sure why you changed EAGAIN to > EINVAL, and it's obviously a bit ugly, but it looks like a valid fix > for what the SLUB_TYPESAFE_BY_RCU conversion broke. > > Reviewed-by: Jann Horn <jannh@google.com> > > > But as a side note, I am a bit confused about how the concurrency of > the existing code is supposed to work... it looks like two parallel > calls to mmap_singleton() can end up returning different pointers, and > then the singleton is not actually a singleton anymore? If that part > of the existing code is wrong even before the SLAB_TYPESAFE_BY_RCU > conversion, we might later have to open-code get_file_rcu() in > mmap_singleton(), so that we can do a cmpxchg at the end that checks > whether the i915->gem.mmap_singleton pointer is still the same? > > Like: > > static struct file *mmap_singleton(struct drm_i915_private *i915) > { > struct file *file, *new_file; > > rcu_read_lock(); > file = READ_ONCE(i915->gem.mmap_singleton); > if (file && atomic_long_inc_not_zero(&file->f_count)) { > rcu_read_unlock(); > return file; > } > rcu_read_unlock(); > > new_file = anon_inode_getfile("i915.gem", &singleton_fops, > i915, O_RDWR); > if (IS_ERR(new_file)) > return new_file; > > /* Everyone shares a single global address space */ > new_file->f_mapping = i915->drm.anon_inode->i_mapping; > > if (try_cmpxchg(&i915->gem.mmap_singleton, &file, new_file)) { > // something with drm_dev refcount ? > return new_file; > } > > // something with drm_dev refcount ? > fput(new_file); > > return file; > } > > It would be nice to get some i915 maintainer's comment on how the > singleton stuff is supposed to work. I see that the story has mostly been unraveled by now elsewhere in the thread, and yes, upon some historical digging and looking at the code I agree that the name singleton is confusing/misleading. And although race can happen and we can end up with more than one anonymous inode, I don't think there is any real harm which would warrant complicating this code further. Perhaps a good enough solution would be to rename into something like i915->gem.cached_mmap_anon and put a comment above the smp_store_mb explaining that the race is unlikely but harmless. Because all i915 needs is to decouple the file refcount from the DRM file, and whether it manages to do it with one anon file, or rarely slightly more than one, it doesn't really matter. So if people agree, plan could be for you to proceed with the get_active_file fix and I can follow up with the rename and comment? Possibly at some later date so to avoid conflicts between the trees. Regards, Tvrtko > >> --- >> >> Jann, Linus, >> >> Since you've been quite involved, can you check that what I'm babbling >> here makes sense? If this isn't the fix then I would have to drop the >> SLAB_TYPESAFE_BY_RCU conversion patch for now since I'd like to send PRs >> by the end of the week. >> >> This is on top of >> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.misc >> >> Appreciate the help, thanks! >> Christian >> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- >> fs/file.c | 17 ++++++++++++++++- >> include/linux/fs.h | 1 + >> 3 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c >> index 97bf10861cad..da97e61b18d4 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c >> @@ -916,7 +916,7 @@ static struct file *mmap_singleton(struct drm_i915_private *i915) >> struct file *file; >> >> rcu_read_lock(); >> - file = get_file_rcu(&i915->gem.mmap_singleton); >> + file = get_file_rcu_once(&i915->gem.mmap_singleton); >> rcu_read_unlock(); >> if (file) >> return file; >> diff --git a/fs/file.c b/fs/file.c >> index 1a475d7d636e..2c64d6836f0c 100644 >> --- a/fs/file.c >> +++ b/fs/file.c >> @@ -867,7 +867,7 @@ static struct file *__get_file_rcu(struct file __rcu **f) >> return NULL; >> >> if (unlikely(!atomic_long_inc_not_zero(&file->f_count))) >> - return ERR_PTR(-EAGAIN); >> + return ERR_PTR(-EINVAL); >> >> file_reloaded = rcu_dereference_raw(*f); >> >> @@ -927,6 +927,21 @@ struct file *get_file_rcu(struct file __rcu **f) >> } >> EXPORT_SYMBOL_GPL(get_file_rcu); >> >> +struct file *get_file_rcu_once(struct file __rcu **f) >> +{ >> + for (;;) { >> + struct file __rcu *file; >> + >> + file = __get_file_rcu(f); >> + if (!IS_ERR(file)) >> + return file; >> + >> + if (PTR_ERR(file) == -EINVAL) >> + return NULL; >> + } >> +} >> +EXPORT_SYMBOL_GPL(get_file_rcu_once); >> + >> static inline struct file *__fget_files_rcu(struct files_struct *files, >> unsigned int fd, fmode_t mask) >> { >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index cb8bfa1f8ecb..657bbd824490 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -1044,6 +1044,7 @@ static inline struct file *get_file(struct file *f) >> } >> >> struct file *get_file_rcu(struct file __rcu **f); >> +struct file *get_file_rcu_once(struct file __rcu **f); >> >> #define file_count(x) atomic_long_read(&(x)->f_count) >> >> -- >> 2.34.1 >>
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 97bf10861cad..da97e61b18d4 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -916,7 +916,7 @@ static struct file *mmap_singleton(struct drm_i915_private *i915) struct file *file; rcu_read_lock(); - file = get_file_rcu(&i915->gem.mmap_singleton); + file = get_file_rcu_once(&i915->gem.mmap_singleton); rcu_read_unlock(); if (file) return file; diff --git a/fs/file.c b/fs/file.c index 1a475d7d636e..2c64d6836f0c 100644 --- a/fs/file.c +++ b/fs/file.c @@ -867,7 +867,7 @@ static struct file *__get_file_rcu(struct file __rcu **f) return NULL; if (unlikely(!atomic_long_inc_not_zero(&file->f_count))) - return ERR_PTR(-EAGAIN); + return ERR_PTR(-EINVAL); file_reloaded = rcu_dereference_raw(*f); @@ -927,6 +927,21 @@ struct file *get_file_rcu(struct file __rcu **f) } EXPORT_SYMBOL_GPL(get_file_rcu); +struct file *get_file_rcu_once(struct file __rcu **f) +{ + for (;;) { + struct file __rcu *file; + + file = __get_file_rcu(f); + if (!IS_ERR(file)) + return file; + + if (PTR_ERR(file) == -EINVAL) + return NULL; + } +} +EXPORT_SYMBOL_GPL(get_file_rcu_once); + static inline struct file *__fget_files_rcu(struct files_struct *files, unsigned int fd, fmode_t mask) { diff --git a/include/linux/fs.h b/include/linux/fs.h index cb8bfa1f8ecb..657bbd824490 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1044,6 +1044,7 @@ static inline struct file *get_file(struct file *f) } struct file *get_file_rcu(struct file __rcu **f); +struct file *get_file_rcu_once(struct file __rcu **f); #define file_count(x) atomic_long_read(&(x)->f_count)
Today we got a report at [1] for rcu stalls on the i915 testsuite in [2] due to the conversion of files to SLAB_TYPSSAFE_BY_RCU. Afaict, get_file_rcu() goes into an infinite loop trying to carefully verify that i915->gem.mmap_singleton hasn't changed - see the splat below. So I stared at this code to figure out what it actually does. It seems that the i915->gem.mmap_singleton pointer itself never had rcu semantics. The i915->gem.mmap_singleton is replaced in file->f_op->release::singleton_release(): static int singleton_release(struct inode *inode, struct file *file) { struct drm_i915_private *i915 = file->private_data; cmpxchg(&i915->gem.mmap_singleton, file, NULL); drm_dev_put(&i915->drm); return 0; } The cmpxchg() is ordered against a concurrent update of i915->gem.mmap_singleton from mmap_singleton(). IOW, when mmap_singleton() fails to get a reference on i915->gem.mmap_singleton via mmap_singleton(): rcu_read_lock(); file = get_file_rcu(&i915->gem.mmap_singleton); rcu_read_unlock(); mmap_singleton() allocates a new file via anon_inode_getfile() and does smp_store_mb(i915->gem.mmap_singleton, file); So, afaiu, then what happens in the case of this bug is that at some point fput() is called and drops the file->f_count to zero but obviously leaving the pointer in i915->gem.mmap_singleton in tact until file->f_op->release::singleton_release() is called. Now, there might be possibly larger delays until file->f_op->release::singleton_release() is called and i915->gem.mmap_singleton is set to NULL? Say concurrently another task hits mmap_singleton() and does: rcu_read_lock(); file = get_file_rcu(&i915->gem.mmap_singleton); rcu_read_unlock(); When get_file_rcu() fails to get a reference via atomic_inc_not_zero() it will try the reload from i915->gem.mmap_singleton assuming it has comparable semantics as __fget_files_rcu() that also reloads on atomic_inc_not_zero() failure. But since i915->gem.mmap_singleton doesn't have proper rcu semantics it reloads the same pointer again, trying the same atomic_inc_not_zero() again and doing so until file->f_op->release::singleton_release() of the old file has been called. So, in contrast to __fget_files_rcu() here we want to not retry when atomic_inc_not_zero() has failed. We only want to retry in case we managed to get a reference but the pointer did change on reload. <3> [511.395679] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks: <3> [511.395716] rcu: Tasks blocked on level-1 rcu_node (CPUs 0-9): P6238 <3> [511.395934] rcu: (detected by 16, t=65002 jiffies, g=123977, q=439 ncpus=20) <6> [511.395944] task:i915_selftest state:R running task stack:10568 pid:6238 tgid:6238 ppid:1001 flags:0x00004002 <6> [511.395962] Call Trace: <6> [511.395966] <TASK> <6> [511.395974] ? __schedule+0x3a8/0xd70 <6> [511.395995] ? asm_sysvec_apic_timer_interrupt+0x1a/0x20 <6> [511.396003] ? lockdep_hardirqs_on+0xc3/0x140 <6> [511.396013] ? asm_sysvec_apic_timer_interrupt+0x1a/0x20 <6> [511.396029] ? get_file_rcu+0x10/0x30 <6> [511.396039] ? get_file_rcu+0x10/0x30 <6> [511.396046] ? i915_gem_object_mmap+0xbc/0x450 [i915] <6> [511.396509] ? i915_gem_mmap+0x272/0x480 [i915] <6> [511.396903] ? mmap_region+0x253/0xb60 <6> [511.396925] ? do_mmap+0x334/0x5c0 <6> [511.396939] ? vm_mmap_pgoff+0x9f/0x1c0 <6> [511.396949] ? rcu_is_watching+0x11/0x50 <6> [511.396962] ? igt_mmap_offset+0xfc/0x110 [i915] <6> [511.397376] ? __igt_mmap+0xb3/0x570 [i915] <6> [511.397762] ? igt_mmap+0x11e/0x150 [i915] <6> [511.398139] ? __trace_bprintk+0x76/0x90 <6> [511.398156] ? __i915_subtests+0xbf/0x240 [i915] <6> [511.398586] ? __pfx___i915_live_setup+0x10/0x10 [i915] <6> [511.399001] ? __pfx___i915_live_teardown+0x10/0x10 [i915] <6> [511.399433] ? __run_selftests+0xbc/0x1a0 [i915] <6> [511.399875] ? i915_live_selftests+0x4b/0x90 [i915] <6> [511.400308] ? i915_pci_probe+0x106/0x200 [i915] <6> [511.400692] ? pci_device_probe+0x95/0x120 <6> [511.400704] ? really_probe+0x164/0x3c0 <6> [511.400715] ? __pfx___driver_attach+0x10/0x10 <6> [511.400722] ? __driver_probe_device+0x73/0x160 <6> [511.400731] ? driver_probe_device+0x19/0xa0 <6> [511.400741] ? __driver_attach+0xb6/0x180 <6> [511.400749] ? __pfx___driver_attach+0x10/0x10 <6> [511.400756] ? bus_for_each_dev+0x77/0xd0 <6> [511.400770] ? bus_add_driver+0x114/0x210 <6> [511.400781] ? driver_register+0x5b/0x110 <6> [511.400791] ? i915_init+0x23/0xc0 [i915] <6> [511.401153] ? __pfx_i915_init+0x10/0x10 [i915] <6> [511.401503] ? do_one_initcall+0x57/0x270 <6> [511.401515] ? rcu_is_watching+0x11/0x50 <6> [511.401521] ? kmalloc_trace+0xa3/0xb0 <6> [511.401532] ? do_init_module+0x5f/0x210 <6> [511.401544] ? load_module+0x1d00/0x1f60 <6> [511.401581] ? init_module_from_file+0x86/0xd0 <6> [511.401590] ? init_module_from_file+0x86/0xd0 <6> [511.401613] ? idempotent_init_module+0x17c/0x230 <6> [511.401639] ? __x64_sys_finit_module+0x56/0xb0 <6> [511.401650] ? do_syscall_64+0x3c/0x90 <6> [511.401659] ? entry_SYSCALL_64_after_hwframe+0x6e/0xd8 <6> [511.401684] </TASK> Link: [1]: https://lore.kernel.org/intel-gfx/SJ1PR11MB6129CB39EED831784C331BAFB9DEA@SJ1PR11MB6129.namprd11.prod.outlook.com Link: [2]: https://intel-gfx-ci.01.org/tree/linux-next/next-20231013/bat-dg2-11/igt@i915_selftest@live@mman.html#dmesg-warnings10963 Signed-off-by: Christian Brauner <brauner@kernel.org> --- Jann, Linus, Since you've been quite involved, can you check that what I'm babbling here makes sense? If this isn't the fix then I would have to drop the SLAB_TYPESAFE_BY_RCU conversion patch for now since I'd like to send PRs by the end of the week. This is on top of https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.misc Appreciate the help, thanks! Christian --- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- fs/file.c | 17 ++++++++++++++++- include/linux/fs.h | 1 + 3 files changed, 18 insertions(+), 2 deletions(-)