On 3/23/21 12:18 AM, Lv Yunlong wrote:
In virtio_fs_get_tree, fm is allocated by kzalloc() and assigned to fsc->s_fs_info by fsc->s_fs_info=fm statement. If the kzalloc() failed, it will goto err directly, so that
Right, I follow this so far.
fsc->s_fs_info must be non-NULL and fm will be freed.
But this I don't follow in the context of the stuff that happens in out_err.
But later fm is freed again when virtio_fs_fill_super() fialed.
I think the statement if (fsc->s_fs_info) {kfree(fm);} is
misplaced.
I'm not sure this can double free, because: * If fm = kzalloc[..] fails, the function bails early. * If sget_fc() fails, the function cleans up fm and fc and bails early.* If sget_fc() succeeds and allocated a new superblock, fc->s_fs_info pointer is moved to sb->s_fs_info and fc->s_fs_info is set to NULL, so the first free hasn't happened yet.
* If sget_fc() succeeds and somehow returns an existing superblock (which I think is tested by checking if fc->s_fs_info is not NULL, since otherwise it'd have been moved to the superblock and set to NULL in sget_fc), I think sb->s_root would not be NULL, therefore the flow of control wouldn't enter the if-block where virtio_fs_fill_super could fail which means the code won't reach the double free.
That's just my reading of it though, and I'm wondering if that makes sense to others :-)
One last comment inline:
My patch puts this statement in the correct palce to avoid double free. Signed-off-by: Lv Yunlong <lyl2...@mail.ustc.edu.cn> --- fs/fuse/virtio_fs.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index 8868ac31a3c0..727cf436828f 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -1437,10 +1437,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc)fsc->s_fs_info = fm;sb = sget_fc(fsc, virtio_fs_test_super, set_anon_super_fc); - if (fsc->s_fs_info) { - fuse_conn_put(fc); - kfree(fm); - } + if (IS_ERR(sb)) return PTR_ERR(sb);
By removing the check from here, it now looks like if sget_fc() fails, then this early return will leak fm's memory and fc's reference.
Connor
@@ -1457,6 +1454,11 @@ static int virtio_fs_get_tree(struct fs_context *fsc)sb->s_flags |= SB_ACTIVE; }+ if (fsc->s_fs_info) {+ fuse_conn_put(fc); + kfree(fm); + } + WARN_ON(fsc->root); fsc->root = dget(sb->s_root); return 0;
