| Message ID | 828bb442-29d0-4bb8-b90d-f200bdd4faf6@web.de (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | packet: Improve exception handling in fanout_add() | expand |
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)
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.
> 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
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.
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)
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.
> 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
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.
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; }