From: Markus Elfring <Markus.Elfring@xxxxxx> Sent: Wednesday, January 10, 2024 2:58 AM > > >> The kfree() function was called in two cases by > >> the create_gpadl_header() 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. > > > > Interestingly, there's a third case in this function where > > "goto nomem" is done, and in this case, msgbody is NULL. > > Does Coccinelle not complain about that case as well? > > > > As I'm sure you know, the code is correct as is, because kfree() > > checks for a NULL argument. So this is really an exercise in > > making Coccinelle happy. To me, the additional label is > > incremental complexity for someone to deal with when > > reading the code at some time in the future. So I'd vote for > > leaving the code as is. But it's not a big deal either way. I > > can see you've been cleaning up a lot of Coccinelle-reported > > issues across the kernel, most of which result in code > > simplifications. If leaving this unchanged causes you problems, > > then I won't object (though perhaps that 3rd "goto nomem" > > should be dealt with as well for consistency). > > How do you think about the clarification approach > “Reconsidering kfree() calls for null pointers (with SmPL)”? > https://lore.kernel.org/cocci/6cbcf640-55e5-2f11-4a09-716fe681c0d2@xxxxxx/ > https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00096.html > It occurred to me overnight that the existing error handling in create_gpadl_header() is unnecessarily complicated. Here's an approach that I think would fix what you have flagged, and would reduce complexity instead of increasing it. Thoughts? Michael diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index 56f7e06c673e..44b1d5c8dfed 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -336,7 +336,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer, sizeof(struct gpa_range) + pfncount * sizeof(u64); msgheader = kzalloc(msgsize, GFP_KERNEL); if (!msgheader) - goto nomem; + return -ENOMEM; INIT_LIST_HEAD(&msgheader->submsglist); msgheader->msgsize = msgsize; @@ -386,8 +386,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer, list_del(&pos->msglistentry); kfree(pos); } - - goto nomem; + kfree(msgheader); + return -ENOMEM; } msgbody->msgsize = msgsize; @@ -416,8 +416,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer, sizeof(struct vmbus_channel_gpadl_header) + sizeof(struct gpa_range) + pagecount * sizeof(u64); msgheader = kzalloc(msgsize, GFP_KERNEL); - if (msgheader == NULL) - goto nomem; + if (!msgheader) + return -ENOMEM; INIT_LIST_HEAD(&msgheader->submsglist); msgheader->msgsize = msgsize; @@ -437,10 +437,6 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer, } return 0; -nomem: - kfree(msgheader); - kfree(msgbody); - return -ENOMEM; } /*
