| Message ID | 5745d81c-3c06-4871-9785-12a469870934@web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | virtiofs: Adjustments for two function implementations | expand |
On Fri, Dec 29, 2023 at 09:38:47AM +0100, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 29 Dec 2023 09:15:07 +0100 > > The kfree() function was called in two cases by > the virtio_fs_get_tree() function during error handling > even if the passed variable contained a null pointer. So what? kfree(NULL) is perfectly acceptable. Are you trying to optimise an error path?
>> The kfree() function was called in two cases by >> the virtio_fs_get_tree() function during error handling >> even if the passed variable contained a null pointer. > > So what? kfree(NULL) is perfectly acceptable. I suggest to reconsider the usefulness of such a special function call. > Are you trying to optimise an error path? I would appreciate if further improvements can be achieved. https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources Regards, Markus
On Fri, Dec 29, 2023 at 10:10:08AM +0100, Markus Elfring wrote: > >> The kfree() function was called in two cases by > >> the virtio_fs_get_tree() function during error handling > >> even if the passed variable contained a null pointer. > > > > So what? kfree(NULL) is perfectly acceptable. > > I suggest to reconsider the usefulness of such a special function call. Can you be more explicit in your suggestion?
>>> So what? kfree(NULL) is perfectly acceptable. >> >> I suggest to reconsider the usefulness of such a special function call. > > Can you be more explicit in your suggestion? I hope that the change acceptance can grow for the presented transformation. Are you looking for an improved patch description? Regards, Markus
On Tue, Jan 02, 2024 at 10:35:17AM +0100, Markus Elfring wrote: > >>> So what? kfree(NULL) is perfectly acceptable. > >> > >> I suggest to reconsider the usefulness of such a special function call. > > > > Can you be more explicit in your suggestion? > > I hope that the change acceptance can grow for the presented transformation. > Are you looking for an improved patch description? Do you consider more clarity in your argumentation?
> Do you consider more clarity in your argumentation?
It is probably clear that the function call “kfree(NULL)” does not perform
data processing which is really useful for the caller.
Such a call is kept in some cases because programmers did not like to invest
development resources for its avoidance.
Regards,
Markus
On Tue, Jan 02, 2024 at 11:47:38AM +0100, Markus Elfring wrote: > > Do you consider more clarity in your argumentation? > > It is probably clear that the function call “kfree(NULL)” does not perform > data processing which is really useful for the caller. > Such a call is kept in some cases because programmers did not like to invest > development resources for its avoidance. on the contrary, it is extremely useful for callers to not have to perform the NULL check themselves. It also mirrors userspace where free(NULL) is valid according to ISO/ANSI C, so eases the transition for programmers who are coming from userspace. It costs nothing in the implementation as it is part of the check for the ZERO_PTR. And from a practical point of view, we can't take it out now. We can never find all the places which assume the current behaviour. So since we must keep kfree(NULL) working, we should take advantage of that to simplify users.
>> It is probably clear that the function call “kfree(NULL)” does not perform >> data processing which is really useful for the caller. >> Such a call is kept in some cases because programmers did not like to invest >> development resources for its avoidance. > > on the contrary, it is extremely useful for callers to not have to perform > the NULL check themselves. Some function calls indicate a resource allocation failure by a null pointer. Such pointer checks are generally performed for error detection so that appropriate exception handling can be chosen. > It also mirrors userspace where free(NULL) > is valid according to ISO/ANSI C, so eases the transition for programmers > who are coming from userspace. It costs nothing in the implementation > as it is part of the check for the ZERO_PTR. How many development efforts do you dare to spend on more complete and efficient error/exception handling? Regards, Markus
On Fri, Dec 29, 2023 at 09:38:47AM +0100, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 29 Dec 2023 09:15:07 +0100 > > The kfree() function was called in two cases by > the virtio_fs_get_tree() function during error handling > even if the passed variable contained a null pointer. > This issue was detected by using the Coccinelle software. > > * Thus use another label. > > * Move an error code assignment into an if branch. > > * Delete an initialisation (for the variable “fc”) > which became unnecessary with this refactoring. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> As Matthew said that kfree(NULL) is perfectly acceptable usage in kernel, so I really don't feel that this patch is required. Current code looks good as it is. Thanks Vivek > --- > fs/fuse/virtio_fs.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > index 2f8ba9254c1e..0746f54ec743 100644 > --- a/fs/fuse/virtio_fs.c > +++ b/fs/fuse/virtio_fs.c > @@ -1415,10 +1415,10 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > { > struct virtio_fs *fs; > struct super_block *sb; > - struct fuse_conn *fc = NULL; > + struct fuse_conn *fc; > struct fuse_mount *fm; > unsigned int virtqueue_size; > - int err = -EIO; > + int err; > > /* This gets a reference on virtio_fs object. This ptr gets installed > * in fc->iq->priv. Once fuse_conn is going away, it calls ->put() > @@ -1431,13 +1431,15 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > } > > virtqueue_size = virtqueue_get_vring_size(fs->vqs[VQ_REQUEST].vq); > - if (WARN_ON(virtqueue_size <= FUSE_HEADER_OVERHEAD)) > - goto out_err; > + if (WARN_ON(virtqueue_size <= FUSE_HEADER_OVERHEAD)) { > + err = -EIO; > + goto lock_mutex; > + } > > err = -ENOMEM; > fc = kzalloc(sizeof(*fc), GFP_KERNEL); > if (!fc) > - goto out_err; > + goto lock_mutex; > > fm = kzalloc(sizeof(*fm), GFP_KERNEL); > if (!fm) > @@ -1476,6 +1478,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > > out_err: > kfree(fc); > +lock_mutex: > mutex_lock(&virtio_fs_mutex); > virtio_fs_put(fs); > mutex_unlock(&virtio_fs_mutex); > -- > 2.43.0 >
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index 2f8ba9254c1e..0746f54ec743 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -1415,10 +1415,10 @@ static int virtio_fs_get_tree(struct fs_context *fsc) { struct virtio_fs *fs; struct super_block *sb; - struct fuse_conn *fc = NULL; + struct fuse_conn *fc; struct fuse_mount *fm; unsigned int virtqueue_size; - int err = -EIO; + int err; /* This gets a reference on virtio_fs object. This ptr gets installed * in fc->iq->priv. Once fuse_conn is going away, it calls ->put() @@ -1431,13 +1431,15 @@ static int virtio_fs_get_tree(struct fs_context *fsc) } virtqueue_size = virtqueue_get_vring_size(fs->vqs[VQ_REQUEST].vq); - if (WARN_ON(virtqueue_size <= FUSE_HEADER_OVERHEAD)) - goto out_err; + if (WARN_ON(virtqueue_size <= FUSE_HEADER_OVERHEAD)) { + err = -EIO; + goto lock_mutex; + } err = -ENOMEM; fc = kzalloc(sizeof(*fc), GFP_KERNEL); if (!fc) - goto out_err; + goto lock_mutex; fm = kzalloc(sizeof(*fm), GFP_KERNEL); if (!fm) @@ -1476,6 +1478,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) out_err: kfree(fc); +lock_mutex: mutex_lock(&virtio_fs_mutex); virtio_fs_put(fs); mutex_unlock(&virtio_fs_mutex);