[bug report] fuse: get rid of fuse_mount refcount

[bug report] fuse: get rid of fuse_mount refcount

[Dan Carpenter]

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

  1452                          sb->s_fs_info = NULL;

I'm sort of surprised this is setting "sb->" instead of "fsc->".

  1453                          deactivate_locked_super(sb);
  1454                          return err;
  1455                  }
  1456  
  1457                  sb->s_flags |= SB_ACTIVE;
  1458          }

regards,
dan carpenter

Re: [bug report] fuse: get rid of fuse_mount refcount

[Miklos Szeredi]

On Fri, Nov 13, 2020 at 10:01 AM Dan Carpenter <dan.carpenter@oracle.com> 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