Re: [RFC][PATCHSET] sorting out RCU-delayed stuff in ->destroy_inode()

[Andreas Dilger <adilger@dilger.ca>]

[-- Attachment #1: Type: text/plain, Size: 2162 bytes --]

On Apr 29, 2019, at 9:09 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> 
> On Tue, Apr 16, 2019 at 11:01:16AM -0700, Linus Torvalds wrote:
>> 
>> I only skimmed through the actual filesystem (and one networking)
>> patches, but they looked like trivial conversions to a better
>> interface.
> 
> ... except that this callback can (and always could) get executed after
> freeing struct super_block.  So we can't just dereference ->i_sb->s_op
> and expect to survive; the table ->s_op pointed to will still be there,
> but ->i_sb might very well have been freed, with all its contents overwritten.
> We need to copy the callback into struct inode itself, unfortunately.
> The following incremental fixes it; I'm going to fold it into the first
> commit in there.
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index fb45590d284e..855dad43b11d 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -164,6 +164,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
> 	inode->i_wb_frn_avg_time = 0;
> 	inode->i_wb_frn_history = 0;
> #endif
> +	inode->free_inode = sb->s_op->free_inode;
> 
> 	if (security_inode_alloc(inode))
> 		goto out;
> @@ -211,8 +212,8 @@ EXPORT_SYMBOL(free_inode_nonrcu);
> static void i_callback(struct rcu_head *head)
> {
> 	struct inode *inode = container_of(head, struct inode, i_rcu);
> -	if (inode->i_sb->s_op->free_inode)
> -		inode->i_sb->s_op->free_inode(inode);
> +	if (inode->free_inode)
> +		inode->free_inode(inode);
> 	else
> 		free_inode_nonrcu(inode);
> }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2e9b9f87caca..5ed6b39e588e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -718,6 +718,7 @@ struct inode {
> #endif
> 
> 	void			*i_private; /* fs or device private pointer */
> +	void (*free_inode)(struct inode *);

It seems like a waste to increase the size of every struct inode just to access
a static pointer.  Is this the only place that ->free_inode() is called?  Why
not move the ->free_inode() pointer into inode->i_fop->free_inode() so that it
is still directly accessible at this point.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]