On Wed, Oct 25, 2023 at 2:01 PM Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> Link: [2]: https://intel-gfx-ci.01.org/tree/linux-next/next-20231013/bat-dg2-11/igt@i915_selftest@live@xxxxxxxxx#dmesg-warnings10963
> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
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@xxxxxxxxxx>
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
>
