On Fri, Nov 13, 2020 at 10:01 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Miklos Szeredi,
>
> The patch 514b5e3ff45e: "fuse: get rid of fuse_mount refcount" from
> Nov 11, 2020, leads to the following static checker warning:
>
> fs/fuse/virtio_fs.c:1451 virtio_fs_get_tree()
> error: double free of 'fm'
>
> fs/fuse/virtio_fs.c
> 1418 if (!fs) {
> 1419 pr_info("virtio-fs: tag <%s> not found\n", fsc->source);
> 1420 return -EINVAL;
> 1421 }
> 1422
> 1423 err = -ENOMEM;
> 1424 fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
> 1425 if (!fc)
> 1426 goto out_err;
> 1427
> 1428 fm = kzalloc(sizeof(struct fuse_mount), GFP_KERNEL);
> 1429 if (!fm)
> 1430 goto out_err;
> 1431
> 1432 fuse_conn_init(fc, fm, get_user_ns(current_user_ns()),
> 1433 &virtio_fs_fiq_ops, fs);
> 1434 fc->release = fuse_free_conn;
> 1435 fc->delete_stale = true;
> 1436 fc->auto_submounts = true;
> 1437
> 1438 fsc->s_fs_info = fm;
> 1439 sb = sget_fc(fsc, virtio_fs_test_super, set_anon_super_fc);
> 1440 if (fsc->s_fs_info) {
> 1441 fuse_conn_put(fc);
> 1442 kfree(fm);
> ^^^^^^^^^
> Freed here
>
> 1443 }
> 1444 if (IS_ERR(sb))
> 1445 return PTR_ERR(sb);
> 1446
> 1447 if (!sb->s_root) {
> 1448 err = virtio_fs_fill_super(sb, fsc);
> 1449 if (err) {
> 1450 fuse_conn_put(fc);
> 1451 kfree(fm);
> ^^^^^^^^^
> Double free
The code is correct but tricky to prove. So here it goes:
We set fsc->s_fs_info to non-NULL before calling sget_fc().
sget_fc() will set fsc->s_fs_info to NULL only if we return a newly
allocated superblock (i.e. sb->s_root is NULL).
In case sget_fc() returns an old superblock, then that means
sb->s_root is non-NULL. To prove this look at grab_super() which
checks SB_BORN. SB_BORN is set in vfs_get_tree() after a successful
call to fsc->ops->get_tree() (i.e. virtio_fs_get_tree()), which in
turn will return success only if sb->s_root has been set to non-NULL
(see virtio_fs_fill_super() and fuse_fill_super_common()).
Now we know that sget_fc() will return with
(a negative fsc->s_fs_info AND a negative sb->s_root) OR
(a positive fsc->s_fs_info AND a (positive sb->s_root OR an error))
So the double free can never happen. I wouldn't expect the static
checker to figure that out.
>
> 1452 sb->s_fs_info = NULL;
>
> I'm sort of surprised this is setting "sb->" instead of "fsc->".
The reason is that deactivate_locked_super() will call ->kill_sb (i.e.
virtio_kill_sb()) and we don't want that function to mess with the
destruction of a half baked fuse_mount structure. So we just free it
by hand and set sb->s_fs_info to NULL.
Thanks,
Miklos
