[PATCH] packet: Improve exception handling in fanout_add()

[PATCH] packet: Improve exception handling in fanout_add()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 31 Dec 2023 16:30:51 +0100

The kfree() function was called in some cases by the fanout_add() function
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.

Thus use another label.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 net/packet/af_packet.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 5f1757a32842..0681d4f1ed85 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1712,14 +1712,14 @@ static int fanout_add(struct sock *sk, struct fanout_args *args)

 	err = -EALREADY;
 	if (po->fanout)
-		goto out;
+		goto unlock_mutex;

 	if (type == PACKET_FANOUT_ROLLOVER ||
 	    (type_flags & PACKET_FANOUT_FLAG_ROLLOVER)) {
 		err = -ENOMEM;
 		rollover = kzalloc(sizeof(*rollover), GFP_KERNEL);
 		if (!rollover)
-			goto out;
+			goto unlock_mutex;
 		atomic_long_set(&rollover->num, 0);
 		atomic_long_set(&rollover->num_huge, 0);
 		atomic_long_set(&rollover->num_failed, 0);
@@ -1812,6 +1812,7 @@ static int fanout_add(struct sock *sk, struct fanout_args *args)

 out:
 	kfree(rollover);
+unlock_mutex:
 	mutex_unlock(&fanout_mutex);
 	return err;
 }
--
2.43.0

Re: [PATCH] packet: Improve exception handling in fanout_add()

[Willem de Bruijn]

Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 31 Dec 2023 16:30:51 +0100
> 
> The kfree() function was called in some cases by the fanout_add() function
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.

It is fine to call kfree with a possible NULL pointer:

	/**
	 * kfree - free previously allocated memory
	 * @object: pointer returned by kmalloc() or kmem_cache_alloc()
	 *
	 * If @object is NULL, no operation is performed.
	 */
	void kfree(const void *object)

Re: packet: Improve exception handling in fanout_add()

[Markus Elfring]

> It is fine to call kfree with a possible NULL pointer:
…
> 	 * If @object is NULL, no operation is performed.
> 	 */
> 	void kfree(const void *object)

Such a function call triggers an input parameter validation
with a corresponding immediate return, doesn't it?
Do you find such data processing really helpful for the desired error/exception handling?

Regards,
Markus

Re: packet: Improve exception handling in fanout_add()

[Willem de Bruijn]

Markus Elfring wrote:
> > It is fine to call kfree with a possible NULL pointer:
> …
> > 	 * If @object is NULL, no operation is performed.
> > 	 */
> > 	void kfree(const void *object)
> 
> Such a function call triggers an input parameter validation
> with a corresponding immediate return, doesn't it?
> Do you find such data processing really helpful for the desired error/exception handling?

It's not just personal preference. It is an established pattern to
avoid extra NULL tests around kfree.

A quick git log to show a few recent examples of patches that expressly
remove such branches, e.g.,

commit d0110443cf4a ("amd/pds_core: core: No need for Null pointer check before kfree")
commit efd9d065de67 ("drm/radeon: Remove unnecessary NULL test before kfree in 'radeon_connector_free_edid'")

An interesting older thread on the topic:

https://linux-kernel.vger.kernel.narkive.com/KVjlDsTo/kfree-null

My summary, the many branches scattered throughout the kernel likely
are more expensive than the occasional save from seeing the rare NULL
pointer.

RE: packet: Improve exception handling in fanout_add()

[David Laight]

From: Willem de Bruijn
> Sent: 01 January 2024 15:29
> 
> Markus Elfring wrote:
> > > It is fine to call kfree with a possible NULL pointer:
> > …
> > > 	 * If @object is NULL, no operation is performed.
> > > 	 */
> > > 	void kfree(const void *object)
> >
> > Such a function call triggers an input parameter validation
> > with a corresponding immediate return, doesn't it?
> > Do you find such data processing really helpful for the desired error/exception handling?
> 
> It's not just personal preference. It is an established pattern to
> avoid extra NULL tests around kfree.
> 
> A quick git log to show a few recent examples of patches that expressly
> remove such branches, e.g.,
> 
> commit d0110443cf4a ("amd/pds_core: core: No need for Null pointer check before kfree")
> commit efd9d065de67 ("drm/radeon: Remove unnecessary NULL test before kfree in
> 'radeon_connector_free_edid'")
> 
> An interesting older thread on the topic:
> 
> https://linux-kernel.vger.kernel.narkive.com/KVjlDsTo/kfree-null

That actually fails to note that if the call site contains the
conditional (explicitly or from a #define/static inline) then gcc
will optimise the test away if it can determine that the pointer
is NULL (from an earlier test) or non-NULL (has been dereferenced).
Possibly because gcc didn't do that 18 years ago.

> My summary, the many branches scattered throughout the kernel likely
> are more expensive than the occasional save from seeing the rare NULL
> pointer.

Especially in error paths or where the normal case is that the pointer
is allocated.

About the only place where a check in the caller may make sense is
for frequently used code paths where the pointer is normally NULL.
One example would be the code that reads an iovec[] from user space.
An on-stack array is used for small (sane) fragment counts with
kmalloc() being called for large counts.
In that case having 'if (unlikely(ptr)) kfree(ptr);' will probably
generate code that is measurably faster.
(Especially if there are mitigations for 'ret'.)

I also believe that likely/unlikely have almost no effect on modern x86.
(was it the P-Pro that used prefix for static prediction?)
IIRC there is no 'static prediction' - prediction is based on whatever
'set of branches' the current branch alases to.
The only slight gain is that the 'fall through' path is less likely
to stall due to a cache miss. But even that can be far enough ahead
of the non-speculative execution point that the actual stall is small.

Of course other simpler cpu do have static prediction rules.
Common would be to predict backwards branches taken (eg loops)
and forwards ones not-taken.
If you really do care (especially if you've disabled any dynamic prediction
in order to get measurable execution times) then you can need to add
(eg) asm comments to force a predicted not-taken forwards branch to
an unconditional backwards branch to avoid a 'predicted taken' backward
branch.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: packet: Improve exception handling in fanout_add()

[Stephen Hemminger]

On Mon, 1 Jan 2024 10:46:45 +0100
Markus Elfring <Markus.Elfring@web.de> wrote:

> > It is fine to call kfree with a possible NULL pointer:  
> …
> > 	 * If @object is NULL, no operation is performed.
> > 	 */
> > 	void kfree(const void *object)  
> 
> Such a function call triggers an input parameter validation
> with a corresponding immediate return, doesn't it?
> Do you find such data processing really helpful for the desired error/exception handling?

If you look at the existing coccinelle script there is even one
to remove unnecessary checks for null before calling kfree.

Re: packet: Improve exception handling in fanout_add()

[Markus Elfring]

> If you look at the existing coccinelle script there is even one
> to remove unnecessary checks for null before calling kfree.

The avoidance of extra pointer checks is an other use case than
omitting redundant function calls, isn't it?

Regards,
Markus

Re: [PATCH] packet: Improve exception handling in fanout_add()

[Stephen Hemminger]

On Sun, 31 Dec 2023 16:39:02 +0100
Markus Elfring <Markus.Elfring@web.de> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 31 Dec 2023 16:30:51 +0100
> 
> The kfree() function was called in some cases by the fanout_add() function
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
> 
> Thus use another label.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

NAK
kfree of null is perfectly fine. 
There is no need for this patch.

Re: [PATCH] packet: Improve exception handling in fanout_add()

[Stephen Hemminger]

On Sun, 31 Dec 2023 16:39:02 +0100
Markus Elfring <Markus.Elfring@web.de> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 31 Dec 2023 16:30:51 +0100
> 
> The kfree() function was called in some cases by the fanout_add() function
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
> 
> Thus use another label.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---

Since you are seem to not listen to feedback from others,
I hope this patch is just ignored.