Page MenuHomeFreeBSD

LinuxKPI: Implement get_file_rcu()
ClosedPublic

Authored by wulf on Aug 24 2021, 11:48 PM.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wulf requested review of this revision.Aug 24 2021, 11:48 PM
This revision is now accepted and ready to land.Aug 25 2021, 9:20 AM
hselasky added inline comments.
sys/compat/linuxkpi/common/include/linux/fs.h
258

This change alone is incorrect! Please ensure that all structures being accessed by this function is freed by kfree_rcu()!

f_count being zero only happens when the Linux file is freed.

But neither "struct linux_file" nor "struct file" is currently freed by RCU.

That means this function will introduce use-after-free! And is not safe to use.

This revision now requires changes to proceed.Aug 25 2021, 9:41 AM

markj@ How can we solve this w/o sleepable EPOCH in the kernel?

Maybe we need a custom "free" file operation?

sys/compat/linuxkpi/common/include/linux/fs.h
258

This change alone is incorrect! Please ensure that all structures being accessed by this function is freed by kfree_rcu()!

As I see, release handler of this file uses ordinary kfree(), not kfree_rcu()

f_count being zero only happens when the Linux file is freed.

But neither "struct linux_file" nor "struct file" is currently freed by RCU.

That means this function will introduce use-after-free! And is not safe to use.

Do you mean that linux_file_close() content should be replaced with single call_rcu() with current it's content moved to call_rcu's callback?

add rcu_barrier()-s to file close handler to wait for RCU callbacks
scheduled earlier by other file operations.

sys/compat/linuxkpi/common/src/linux_compat.c
1543

I think the right place to put the barrier is right before kfree(), because the release function is important removing the file entry from the list which the DRM driver is iterating.

Also you don't need a barrier on the SLEEPABLE type.

Place barrier right before kfree().
Remove barrier of the SLEEPABLE type.

sys/compat/linuxkpi/common/src/linux_compat.c
1541

Looking closer at this you need to call:

synchronize_rcu()

because the protection given by rcu_barrier is not strong enough.

May be call kfree_rcu() with struct rcu_head embedded into struct linux_file?

sys/compat/linuxkpi/common/include/linux/fs.h
258

This change alone is incorrect! Please ensure that all structures being accessed by this function is freed by kfree_rcu()!

As I see, release handler of this file uses ordinary kfree(), not kfree_rcu()

f_count being zero only happens when the Linux file is freed.

But neither "struct linux_file" nor "struct file" is currently freed by RCU.

That means this function will introduce use-after-free! And is not safe to use.

Do you mean that linux_file_close() content should be replaced with single call_rcu() with current it's content moved to call_rcu's callback?

Replace rcu_barrier() with synchronize_rcu()

Looks good.

Please test that the code works before submitting.

This revision is now accepted and ready to land.Aug 25 2021, 8:25 PM

Looks good.

Please test that the code works before submitting.

It works for me both ways: with and without the barrier.
Some sort of stress test is required to test the code for races.

Wait for a grace period when anonymous file is closed with fput().

As this case happens in drm-kmod very frequently, free memory allocated
by struct file asynchronously with kfree_rcu().

This revision now requires review to proceed.Thu, Sep 23, 12:52 AM

Are you sure this works, that other structure pointers inside the Linux file structure won't be accessed and will also need to be freed by RCU?

Are you sure this works, that other structure pointers inside the Linux file structure won't be accessed and will also need to be freed by RCU?

If I understand Linux code correctly, one must execute get_file_rcu() with RCU read lock taken[1] before access to any other structure member.
In that case success of get_file_rcu() prevents execution of kfree(struct *file)

[1] https://elixir.bootlin.com/linux/v5.15-rc2/source/drivers/gpu/drm/i915/gem/i915_gem_mman.c#L874

Hi,

The problem is that the refcount mechanism access f->_file->f_count . You need to free both "f" and "_file" using kfree_rcu() for this to work.
"f" is the Linux file structure and "_file" is the FreeBSD kernel's file structure.

What was the problem with the previous version of this patch? Performance?

--HPS

Revert close() handler to previous state

The problem is that the refcount mechanism access f->_file->f_count . You need to free both "f" and "_file" using kfree_rcu() for this to work.
"f" is the Linux file structure and "_file" is the FreeBSD kernel's file structure.

Somehow I overlooked that. Reverted.

What was the problem with the previous version of this patch? Performance?

It missed support for files allocated with shmem_file_setup() and anon_inode_getfile(). They both have filp->_file equal to NULL, so I left kfree_rcu here.

I only see a partial patch revert.

This revision is now accepted and ready to land.Thu, Sep 23, 1:48 PM

Yes, it is partial. kfree_rcu in linux_file_free() is new as compared with old version.

This revision was automatically updated to reflect the committed changes.