| Message ID | 20231116075726.28634-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: fuse: dax: set fc->dax to NULL in fuse_dax_conn_free() | expand |
On 11/16/23 3:57 PM, Hangyu Hua wrote: > fuse_dax_conn_free() will be called when fuse_fill_super_common() fails > after fuse_dax_conn_alloc(). Then deactivate_locked_super() in > virtio_fs_get_tree() will call virtio_kill_sb() to release the discarded > superblock. This will call fuse_dax_conn_free() again in fuse_conn_put(), > resulting in a possible double free. > > Fixes: 1dd539577c42 ("virtiofs: add a mount option to enable dax") > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> Reviewed-by: Jingbo Xu <jefflexu@linux.alibaba.com> > --- > fs/fuse/dax.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c > index 23904a6a9a96..12ef91d170bb 100644 > --- a/fs/fuse/dax.c > +++ b/fs/fuse/dax.c > @@ -1222,6 +1222,7 @@ void fuse_dax_conn_free(struct fuse_conn *fc) > if (fc->dax) { > fuse_free_dax_mem_ranges(&fc->dax->free_ranges); > kfree(fc->dax); > + fc->dax = NULL; > } > } >
On Thu, 16 Nov 2023 at 08:57, Hangyu Hua <hbh25y@gmail.com> wrote: > > fuse_dax_conn_free() will be called when fuse_fill_super_common() fails > after fuse_dax_conn_alloc(). Then deactivate_locked_super() in > virtio_fs_get_tree() will call virtio_kill_sb() to release the discarded > superblock. This will call fuse_dax_conn_free() again in fuse_conn_put(), > resulting in a possible double free. > > Fixes: 1dd539577c42 ("virtiofs: add a mount option to enable dax") > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > --- > fs/fuse/dax.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c > index 23904a6a9a96..12ef91d170bb 100644 > --- a/fs/fuse/dax.c > +++ b/fs/fuse/dax.c > @@ -1222,6 +1222,7 @@ void fuse_dax_conn_free(struct fuse_conn *fc) > if (fc->dax) { > fuse_free_dax_mem_ranges(&fc->dax->free_ranges); > kfree(fc->dax); > + fc->dax = NULL; Is there a reason not to simply remove the fuse_dax_conn_free() call from the cleanup path in fuse_fill_super_common()? Thanks, Miklos > } > } > > -- > 2.34.1 >
On 30/11/2023 18:54, Miklos Szeredi wrote: > On Thu, 16 Nov 2023 at 08:57, Hangyu Hua <hbh25y@gmail.com> wrote: >> >> fuse_dax_conn_free() will be called when fuse_fill_super_common() fails >> after fuse_dax_conn_alloc(). Then deactivate_locked_super() in >> virtio_fs_get_tree() will call virtio_kill_sb() to release the discarded >> superblock. This will call fuse_dax_conn_free() again in fuse_conn_put(), >> resulting in a possible double free. >> >> Fixes: 1dd539577c42 ("virtiofs: add a mount option to enable dax") >> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> >> --- >> fs/fuse/dax.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c >> index 23904a6a9a96..12ef91d170bb 100644 >> --- a/fs/fuse/dax.c >> +++ b/fs/fuse/dax.c >> @@ -1222,6 +1222,7 @@ void fuse_dax_conn_free(struct fuse_conn *fc) >> if (fc->dax) { >> fuse_free_dax_mem_ranges(&fc->dax->free_ranges); >> kfree(fc->dax); >> + fc->dax = NULL; > > Is there a reason not to simply remove the fuse_dax_conn_free() call > from the cleanup path in fuse_fill_super_common()? I think setting fc->dax to NULL keeps the memory allocation and release functions together in fuse_fill_super_common more readable. What do you think? Thanks, Hangyu > > Thanks, > Miklos > > >> } >> } >> >> -- >> 2.34.1 >>
On Fri, Dec 01, 2023 at 10:42:53AM +0800, Hangyu Hua wrote: > On 30/11/2023 18:54, Miklos Szeredi wrote: > > On Thu, 16 Nov 2023 at 08:57, Hangyu Hua <hbh25y@gmail.com> wrote: > > > > > > fuse_dax_conn_free() will be called when fuse_fill_super_common() fails > > > after fuse_dax_conn_alloc(). Then deactivate_locked_super() in > > > virtio_fs_get_tree() will call virtio_kill_sb() to release the discarded > > > superblock. This will call fuse_dax_conn_free() again in fuse_conn_put(), > > > resulting in a possible double free. > > > > > > Fixes: 1dd539577c42 ("virtiofs: add a mount option to enable dax") > > > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > > > --- > > > fs/fuse/dax.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c > > > index 23904a6a9a96..12ef91d170bb 100644 > > > --- a/fs/fuse/dax.c > > > +++ b/fs/fuse/dax.c > > > @@ -1222,6 +1222,7 @@ void fuse_dax_conn_free(struct fuse_conn *fc) > > > if (fc->dax) { > > > fuse_free_dax_mem_ranges(&fc->dax->free_ranges); > > > kfree(fc->dax); > > > + fc->dax = NULL; > > > > Is there a reason not to simply remove the fuse_dax_conn_free() call > > from the cleanup path in fuse_fill_super_common()? > > I think setting fc->dax to NULL keeps the memory allocation and release > functions together in fuse_fill_super_common more readable. What do you > think? I agree with this. fuse_fill_super_common() calls fuse_dax_conn_alloc() which in-turn initializes fc->dax. If fuse_fill_super_common() fails later after calling fuse_dax_conn_alloc(), then cleanup of fc->dax and other associated stuff in same function makes sense. As a code reader I would like to know how fc->dax is being freed in case of error and its right there in the error path (err_free_dax:). I think I set the fc->dax upon initialization. Upon failure I freed the data structures but did not set fc->dax back to NULL. To me, this patch looks reasonable. Acked-by: Vivek Goyal <vgoyal@redhat.com> Thanks Vivek > > Thanks, > Hangyu > > > > > Thanks, > > Miklos > > > > > > > } > > > } > > > > > > -- > > > 2.34.1 > > > >
On Fri, 1 Dec 2023 at 20:08, Vivek Goyal <vgoyal@redhat.com> wrote: > > On Fri, Dec 01, 2023 at 10:42:53AM +0800, Hangyu Hua wrote: > > I think setting fc->dax to NULL keeps the memory allocation and release > > functions together in fuse_fill_super_common more readable. What do you > > think? > > I agree with this. fuse_fill_super_common() calls fuse_dax_conn_alloc() > which in-turn initializes fc->dax. If fuse_fill_super_common() fails > later after calling fuse_dax_conn_alloc(), then cleanup of fc->dax > and other associated stuff in same function makes sense. > > As a code reader I would like to know how fc->dax is being freed in > case of error and its right there in the error path (err_free_dax:). > > I think I set the fc->dax upon initialization. Upon failure I freed > the data structures but did not set fc->dax back to NULL. > > To me, this patch looks reasonable. > > Acked-by: Vivek Goyal <vgoyal@redhat.com> Applied, thanks. The patch is clearly correct, so I have not major issue with it. It just bothers me a little that we have two cleanup instances when we only need one, but I bow to the opinion of the majority. Thanks, Miklos
diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c index 23904a6a9a96..12ef91d170bb 100644 --- a/fs/fuse/dax.c +++ b/fs/fuse/dax.c @@ -1222,6 +1222,7 @@ void fuse_dax_conn_free(struct fuse_conn *fc) if (fc->dax) { fuse_free_dax_mem_ranges(&fc->dax->free_ranges); kfree(fc->dax); + fc->dax = NULL; } }
fuse_dax_conn_free() will be called when fuse_fill_super_common() fails after fuse_dax_conn_alloc(). Then deactivate_locked_super() in virtio_fs_get_tree() will call virtio_kill_sb() to release the discarded superblock. This will call fuse_dax_conn_free() again in fuse_conn_put(), resulting in a possible double free. Fixes: 1dd539577c42 ("virtiofs: add a mount option to enable dax") Signed-off-by: Hangyu Hua <hbh25y@gmail.com> --- fs/fuse/dax.c | 1 + 1 file changed, 1 insertion(+)