From: Markus Elfring <[email protected]>
Date: Fri, 29 Dec 2023 09:28:09 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Improve three size determinations
Improve error handling in virtio_fs_get_tree()
fs/fuse/virtio_fs.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Fri, 29 Dec 2023 08:42:04 +0100
Replace the specification of data structures by pointer dereferences
as the parameter for the operator “sizeof” to make the corresponding size
determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
fs/fuse/virtio_fs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 5f1be1da92ce..2f8ba9254c1e 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1435,11 +1435,11 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
goto out_err;
err = -ENOMEM;
- fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
+ fc = kzalloc(sizeof(*fc), GFP_KERNEL);
if (!fc)
goto out_err;
- fm = kzalloc(sizeof(struct fuse_mount), GFP_KERNEL);
+ fm = kzalloc(sizeof(*fm), GFP_KERNEL);
if (!fm)
goto out_err;
@@ -1495,7 +1495,7 @@ static int virtio_fs_init_fs_context(struct fs_context *fsc)
if (fsc->purpose == FS_CONTEXT_FOR_SUBMOUNT)
return fuse_init_fs_context_submount(fsc);
- ctx = kzalloc(sizeof(struct fuse_fs_context), GFP_KERNEL);
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
fsc->fs_private = ctx;
--
2.43.0
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
On Fri, Dec 29, 2023 at 09:38:47AM +0100, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
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
>
On Fri, Dec 29, 2023 at 09:36:36AM +0100, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Fri, 29 Dec 2023 08:42:04 +0100
>
> Replace the specification of data structures by pointer dereferences
> as the parameter for the operator “sizeof” to make the corresponding size
> determination a bit safer according to the Linux coding style convention.
I had a look at coding-style.rst and it does say that dereferencing the
pointer is preferred form. Primary argument seems to be that somebody
might change the pointer variable type but not the corresponding type
passed to sizeof().
There is some value to the argument. I don't feel strongly about it.
Miklos, if you like this change, feel free to apply.
Thanks
Vivek
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> fs/fuse/virtio_fs.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 5f1be1da92ce..2f8ba9254c1e 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -1435,11 +1435,11 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
> goto out_err;
>
> err = -ENOMEM;
> - fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
> + fc = kzalloc(sizeof(*fc), GFP_KERNEL);
> if (!fc)
> goto out_err;
>
> - fm = kzalloc(sizeof(struct fuse_mount), GFP_KERNEL);
> + fm = kzalloc(sizeof(*fm), GFP_KERNEL);
> if (!fm)
> goto out_err;
>
> @@ -1495,7 +1495,7 @@ static int virtio_fs_init_fs_context(struct fs_context *fsc)
> if (fsc->purpose == FS_CONTEXT_FOR_SUBMOUNT)
> return fuse_init_fs_context_submount(fsc);
>
> - ctx = kzalloc(sizeof(struct fuse_fs_context), GFP_KERNEL);
> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> if (!ctx)
> return -ENOMEM;
> fsc->fs_private = ctx;
> --
> 2.43.0
>
>