On 03/25/2019 12:17 PM, Al Viro wrote:
> On Mon, Mar 25, 2019 at 11:11:23AM +0000, Al Viro wrote:
>> On Mon, Mar 25, 2019 at 10:15:40AM +0100, Daniel Borkmann wrote:
>>>> So we have 5 broken cases, all with the same kind of fix: move freeing
>>>> into the RCU-delayed part of ->destroy_inode(); for debugfs and bpf
>>>> that requires adding ->alloc_inode()/->destroy_inode(), rather than
>>>> relying upon the defaults from fs/inode.c
>>>
>>> I believe I copied that logic from one of the other fs'es back then, sigh.
>>> Thanks for the analysis and hints for fixing. Would the below (only compile
>>> tested for now) look reasonable to you? I believe there is no other way
>>> around than to add our own inode cache, but so be it. The freeing of the
>>> i_link is then RCU-deferred in bpf_destroy_inode_deferred():
>>
>> It looks like it would suffice, but it's way too much boilerplate for my
>> taste ;-/
>>
>> Most of your headache here comes from messing with slab setup and the
>> only reason for that is being unable to free stuff into inode_cachep.
>> So grep for inode_cachep would be a good idea, and it digs up the
>> following sucker:
>> void free_inode_nonrcu(struct inode *inode)
>> {
>> kmem_cache_free(inode_cachep, inode);
>> }
>> EXPORT_SYMBOL(free_inode_nonrcu);
>>
>> IOW, you need nothing on ->alloc_inode() side and for ->destroy_inode()
>> just do call_rcu() of a callback, that would kfree(inode->link) if
>> it was a symlink, then call free_inode_nonrcu(inode).
>>
>> Considerably smaller patch that way...
>
> AFAICS, it boils down to just this (also only compile-tested):
Ah yep, much better, agree. Want to cook proper patch for bpf, or want
me to take care of it?
Thanks,
Daniel
> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> index 2ada5e21dfa6..9437f88b6acf 100644
> --- a/kernel/bpf/inode.c
> +++ b/kernel/bpf/inode.c
> @@ -80,6 +80,20 @@ static const struct inode_operations bpf_dir_iops;
> static const struct inode_operations bpf_prog_iops = { };
> static const struct inode_operations bpf_map_iops = { };
>
> +static void bpf_destroy_inode_deferred(struct rcu_head *head)
> +{
> + struct inode *inode = container_of(head, struct inode, i_rcu);
> +
> + if (S_ISLNK(inode->i_mode))
> + kfree(inode->i_link);
> + free_inode_nonrcu(inode);
> +}
> +
> +static void bpf_destroy_inode(struct inode *inode)
> +{
> + call_rcu(&inode->i_rcu, bpf_destroy_inode_deferred);
> +}
> +
> static struct inode *bpf_get_inode(struct super_block *sb,
> const struct inode *dir,
> umode_t mode)
> @@ -561,8 +575,6 @@ static void bpf_evict_inode(struct inode *inode)
> truncate_inode_pages_final(&inode->i_data);
> clear_inode(inode);
>
> - if (S_ISLNK(inode->i_mode))
> - kfree(inode->i_link);
> if (!bpf_inode_type(inode, &type))
> bpf_any_put(inode->i_private, type);
> }
> @@ -584,6 +596,7 @@ static const struct super_operations bpf_super_ops = {
> .drop_inode = generic_delete_inode,
> .show_options = bpf_show_options,
> .evict_inode = bpf_evict_inode,
> + .destroy_inode = bpf_destroy_inode,
> };
>
> enum {
>
