LinuxKPI: Implement get_file_rcu()

[wulf]

Summary

get_file_rcu() grabs a file if the file->f_count is not zero[1].

[1] https://elixir.bootlin.com/linux/v5.14-rc1/source/include/linux/fs.h#L968

Required by drm-kmod 5.6

[• hselasky]

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

257

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.

[• hselasky]

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

[• hselasky]

Maybe we need a custom "free" file operation?

[wulf]

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

257

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?

[wulf]

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

[• hselasky]

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

1545

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.

[wulf]

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

[• hselasky]

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

1545

Looking closer at this you need to call:

synchronize_rcu()

because the protection given by rcu_barrier is not strong enough.

[wulf]

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

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

257

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?

[wulf]

Replace rcu_barrier() with synchronize_rcu()

[• hselasky]

Looks good.

Please test that the code works before submitting.

[wulf]

In D31672#714703, @hselasky wrote:

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.

[wulf]

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().

[• hselasky]

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?

[wulf]

In D31672#723772, @hselasky wrote:

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

[• hselasky]

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

[wulf]

Revert close() handler to previous state

[wulf]

In D31672#723880, @hselasky wrote:

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.

[• hselasky]

I only see a partial patch revert.

[• hselasky]

Looks good.

[wulf]

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