Re: [PATCH] binfmt_misc: Node could be NULL when evicting inode

[Oleg Nesterov <oleg@redhat.com>]

On 10/10, Eryu Guan wrote:
>
> inode->i_private is assigned by a Node pointer only after
> registering a new binary format, so it could be NULL if we only
> mount binfmt_misc but don't register any format,
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Not really, I 'think... I mean, the problem is that e == NULL when inode
is root_inode, or "status", or "register", created by bm_fill_super()...

Yes, you need to unregister all formats to hit this problem.


Oh, and I'm afraid I intoduced another problem, bm_register_write() error
paths can call iput() with MISC_FMT_OPEN_FILE set but e->interpreter == NULL.
I'll re-check and send another patch today.

Thanks a lot!


Acked-by: Oleg Nesterov <oleg@redhat.com>


> and this results in
> NULL pointer dereference at umount time. e.g.
> 
>   mount -t binfmt_misc binfmt_misc /proc/sys/fs/binfmt_misc
>   umount /proc/sys/fs/binfmt_misc
> 
> [ 9379.678259] BUG: unable to handle kernel NULL pointer dereference at 0000000000000013
> [ 9379.985952] IP: bm_evict_inode+0x16/0x40 [binfmt_misc]
> ...
> [ 9380.964911] Call Trace:
> [ 9380.977633]  evict+0xd3/0x1a0
> [ 9380.994449]  iput+0x17d/0x1d0
> [ 9381.010306]  dentry_unlink_inode+0xb9/0xf0
> [ 9381.034046]  __dentry_kill+0xc7/0x170
> [ 9381.055145]  shrink_dentry_list+0x122/0x280
> [ 9381.078908]  shrink_dcache_parent+0x39/0x90
> [ 9381.103082]  do_one_tree+0x12/0x40
> [ 9381.122005]  shrink_dcache_for_umount+0x2d/0x90
> [ 9381.146517]  generic_shutdown_super+0x1f/0x120
> [ 9381.171644]  kill_litter_super+0x29/0x40
> [ 9381.193513]  deactivate_locked_super+0x43/0x70
> [ 9381.219177]  deactivate_super+0x45/0x60
> [ 9381.240130]  cleanup_mnt+0x3f/0x70
> [ 9381.259064]  __cleanup_mnt+0x12/0x20
> [ 9381.279802]  task_work_run+0x86/0xa0
> [ 9381.299612]  exit_to_usermode_loop+0x6d/0x99
> [ 9381.323872]  syscall_return_slowpath+0xba/0xf0
> [ 9381.350464]  entry_SYSCALL_64_fastpath+0xa3/0xa
> 
> Fix it by making sure Node (e) is not NULL.
> 
> Cc: Oleg Nesterov <oleg@redhat.com>
> Fixes: 83f918274e4b ("exec: binfmt_misc: shift filp_close(interp_file) from kill_node() to bm_evict_inode()")
> Signed-off-by: Eryu Guan <eguan@redhat.com>
> ---
>  fs/binfmt_misc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index 2a46762def31..a7c5a9861bef 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -596,7 +596,7 @@ static void bm_evict_inode(struct inode *inode)
>  {
>  	Node *e = inode->i_private;
>  
> -	if (e->flags & MISC_FMT_OPEN_FILE)
> +	if (e && e->flags & MISC_FMT_OPEN_FILE)
>  		filp_close(e->interp_file, NULL);
>  
>  	clear_inode(inode);
> -- 
> 2.13.6
>