* [PATCH net 4/5] rose: clear neighbour pointer after rose_neigh_put() in state machines
From: Bernard Pidoux @ 2026-04-26 14:43 UTC (permalink / raw)
To: netdev
Cc: linux-hams, linux-kernel, davem, edumazet, kuba, pabeni, horms,
Bernard Pidoux
In-Reply-To: <20260426144305.984349-1-bernard.f6bvp@gmail.com>
After calling rose_neigh_put() in rose_state1_machine() through
rose_state5_machine(), rose->neighbour was left pointing at the
potentially freed neighbour structure. A subsequent timer expiry or
concurrent teardown path could dereference the stale pointer, causing
a use-after-free.
Set rose->neighbour to NULL immediately after each rose_neigh_put()
call in the state machine functions.
Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
Tested-by: Bernard Pidoux <bernard.f6bvp@gmail.com>
Signed-off-by: Bernard Pidoux <bernard.f6bvp@gmail.com>
---
net/rose/rose_in.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/rose/rose_in.c b/net/rose/rose_in.c
index 0276b393f0e5..622527f1354f 100644
--- a/net/rose/rose_in.c
+++ b/net/rose/rose_in.c
@@ -57,6 +57,7 @@ static int rose_state1_machine(struct sock *sk, struct sk_buff *skb, int framety
rose_write_internal(sk, ROSE_CLEAR_CONFIRMATION);
rose_disconnect(sk, ECONNREFUSED, skb->data[3], skb->data[4]);
rose_neigh_put(rose->neighbour);
+ rose->neighbour = NULL;
break;
default:
@@ -80,11 +81,13 @@ static int rose_state2_machine(struct sock *sk, struct sk_buff *skb, int framety
rose_write_internal(sk, ROSE_CLEAR_CONFIRMATION);
rose_disconnect(sk, 0, skb->data[3], skb->data[4]);
rose_neigh_put(rose->neighbour);
+ rose->neighbour = NULL;
break;
case ROSE_CLEAR_CONFIRMATION:
rose_disconnect(sk, 0, -1, -1);
rose_neigh_put(rose->neighbour);
+ rose->neighbour = NULL;
break;
default:
@@ -122,6 +125,7 @@ static int rose_state3_machine(struct sock *sk, struct sk_buff *skb, int framety
rose_write_internal(sk, ROSE_CLEAR_CONFIRMATION);
rose_disconnect(sk, 0, skb->data[3], skb->data[4]);
rose_neigh_put(rose->neighbour);
+ rose->neighbour = NULL;
break;
case ROSE_RR:
@@ -235,6 +239,7 @@ static int rose_state4_machine(struct sock *sk, struct sk_buff *skb, int framety
rose_write_internal(sk, ROSE_CLEAR_CONFIRMATION);
rose_disconnect(sk, 0, skb->data[3], skb->data[4]);
rose_neigh_put(rose->neighbour);
+ rose->neighbour = NULL;
break;
default:
@@ -255,6 +260,7 @@ static int rose_state5_machine(struct sock *sk, struct sk_buff *skb, int framety
rose_write_internal(sk, ROSE_CLEAR_CONFIRMATION);
rose_disconnect(sk, 0, skb->data[3], skb->data[4]);
rose_neigh_put(rose_sk(sk)->neighbour);
+ rose_sk(sk)->neighbour = NULL;
}
return 0;
--
2.51.0
^ permalink raw reply related
* [PATCH net 3/5] rose: fix race between loopback timer and module removal
From: Bernard Pidoux @ 2026-04-26 14:43 UTC (permalink / raw)
To: netdev
Cc: linux-hams, linux-kernel, davem, edumazet, kuba, pabeni, horms,
Bernard Pidoux
In-Reply-To: <20260426144305.984349-1-bernard.f6bvp@gmail.com>
rose_loopback_clear() called timer_delete() which returns immediately
without waiting for any running callback to complete. If the timer
fired concurrently with module removal, rose_loopback_timer() could
re-arm the timer after timer_delete() returned and then access
rose_loopback_neigh after it was freed.
Two complementary changes close the race:
1. Add a loopback_stopping atomic flag. rose_loopback_timer() checks
it at entry (before acquiring a reference) and again inside the
loop; when set it drains the queue and exits without re-arming the
timer.
2. Switch rose_loopback_clear() to timer_delete_sync() so it blocks
until any in-flight callback has returned before freeing resources.
The smp_mb() between setting the flag and calling timer_delete_sync()
ensures the flag is visible to any callback that is about to run.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Tested-by: Bernard Pidoux <bernard.f6bvp@gmail.com>
Signed-off-by: Bernard Pidoux <bernard.f6bvp@gmail.com>
---
net/rose/rose_loopback.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index d66913df360d..80d7879ef36a 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -12,13 +12,15 @@
#include <net/rose.h>
#include <linux/init.h>
-static struct sk_buff_head loopback_queue;
#define ROSE_LOOPBACK_LIMIT 1000
-static struct timer_list loopback_timer;
+static struct timer_list loopback_timer;
+static struct sk_buff_head loopback_queue;
static void rose_set_loopback_timer(void);
static void rose_loopback_timer(struct timer_list *unused);
+static atomic_t loopback_stopping = ATOMIC_INIT(0);
+
void rose_loopback_init(void)
{
skb_queue_head_init(&loopback_queue);
@@ -66,6 +68,9 @@ static void rose_loopback_timer(struct timer_list *unused)
unsigned int lci_i, lci_o;
int count;
+ if (atomic_read(&loopback_stopping))
+ return;
+
if (rose_loopback_neigh)
rose_neigh_hold(rose_loopback_neigh);
else
@@ -75,6 +80,13 @@ static void rose_loopback_timer(struct timer_list *unused)
skb = skb_dequeue(&loopback_queue);
if (!skb)
goto out;
+
+ if (atomic_read(&loopback_stopping)) {
+ kfree_skb(skb);
+ skb_queue_purge(&loopback_queue);
+ goto out;
+ }
+
if (skb->len < ROSE_MIN_LEN) {
kfree_skb(skb);
continue;
@@ -118,7 +130,7 @@ static void rose_loopback_timer(struct timer_list *unused)
out:
rose_neigh_put(rose_loopback_neigh);
- if (!skb_queue_empty(&loopback_queue))
+ if (!atomic_read(&loopback_stopping) && !skb_queue_empty(&loopback_queue))
mod_timer(&loopback_timer, jiffies + 1);
}
@@ -126,10 +138,15 @@ void __exit rose_loopback_clear(void)
{
struct sk_buff *skb;
- timer_delete(&loopback_timer);
+ atomic_set(&loopback_stopping, 1);
+ /* Pairs with atomic_read() in rose_loopback_timer(): ensure the
+ * stopping flag is visible before we cancel, so a concurrent
+ * callback aborts its loop early rather than re-arming the timer.
+ */
+ smp_mb();
+
+ timer_delete_sync(&loopback_timer);
- while ((skb = skb_dequeue(&loopback_queue)) != NULL) {
- skb->sk = NULL;
+ while ((skb = skb_dequeue(&loopback_queue)) != NULL)
kfree_skb(skb);
- }
}
--
2.51.0
^ permalink raw reply related
* [PATCH net 2/5] rose: hold loopback neighbour reference across timer callback
From: Bernard Pidoux @ 2026-04-26 14:43 UTC (permalink / raw)
To: netdev
Cc: linux-hams, linux-kernel, davem, edumazet, kuba, pabeni, horms,
Bernard Pidoux
In-Reply-To: <20260426144305.984349-1-bernard.f6bvp@gmail.com>
rose_loopback_timer() dereferences rose_loopback_neigh throughout its
body but holds no reference on it. A concurrent rose_loopback_clear()
followed by rose_add_loopback_neigh() could free and reallocate the
neighbour while the timer body is running, causing a use-after-free.
Take a reference with rose_neigh_hold() at the start of the callback
(bailing out if the pointer is already NULL) and release it with
rose_neigh_put() at the single exit point. The neigh cannot be freed
while the callback holds a reference.
Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
Tested-by: Bernard Pidoux <bernard.f6bvp@gmail.com>
Signed-off-by: Bernard Pidoux <bernard.f6bvp@gmail.com>
---
net/rose/rose_loopback.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index 914c8f453a1d..d66913df360d 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -66,10 +66,15 @@ static void rose_loopback_timer(struct timer_list *unused)
unsigned int lci_i, lci_o;
int count;
+ if (rose_loopback_neigh)
+ rose_neigh_hold(rose_loopback_neigh);
+ else
+ return;
+
for (count = 0; count < ROSE_LOOPBACK_LIMIT; count++) {
skb = skb_dequeue(&loopback_queue);
if (!skb)
- return;
+ goto out;
if (skb->len < ROSE_MIN_LEN) {
kfree_skb(skb);
continue;
@@ -109,6 +114,10 @@ static void rose_loopback_timer(struct timer_list *unused)
kfree_skb(skb);
}
}
+
+out:
+ rose_neigh_put(rose_loopback_neigh);
+
if (!skb_queue_empty(&loopback_queue))
mod_timer(&loopback_timer, jiffies + 1);
}
--
2.51.0
^ permalink raw reply related
* [PATCH net 1/5] rose: fix dev_put() leak in rose_loopback_timer()
From: Bernard Pidoux @ 2026-04-26 14:43 UTC (permalink / raw)
To: netdev
Cc: linux-hams, linux-kernel, davem, edumazet, kuba, pabeni, horms,
Bernard Pidoux
In-Reply-To: <20260426144305.984349-1-bernard.f6bvp@gmail.com>
rose_rx_call_request() always consumes or returns the skb but never
releases the device reference obtained from rose_dev_get(). When
rose_rx_call_request() succeeds (returns non-zero) dev_put() was never
called, leaking one reference per loopback CALL_REQUEST.
Move dev_put() outside the conditional so it is called unconditionally
after rose_rx_call_request() in all cases.
Also remove the dead check (!rose_loopback_neigh->dev &&
!rose_loopback_neigh->loopback) that immediately precedes it: the
loopback neighbour always has loopback=1 so this condition can never
be true.
Fixes: 0453c6824595 ("net/rose: fix unbound loop in rose_loopback_timer()")
Tested-by: Bernard Pidoux <bernard.f6bvp@gmail.com>
Signed-off-by: Bernard Pidoux <bernard.f6bvp@gmail.com>
---
net/rose/rose_loopback.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index b538e39b3df5..914c8f453a1d 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -96,22 +96,15 @@ static void rose_loopback_timer(struct timer_list *unused)
}
if (frametype == ROSE_CALL_REQUEST) {
- if (!rose_loopback_neigh->dev &&
- !rose_loopback_neigh->loopback) {
- kfree_skb(skb);
- continue;
- }
-
dev = rose_dev_get(dest);
if (!dev) {
kfree_skb(skb);
continue;
}
- if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0) {
- dev_put(dev);
+ if (rose_rx_call_request(skb, dev, rose_loopback_neigh, lci_o) == 0)
kfree_skb(skb);
- }
+ dev_put(dev);
} else {
kfree_skb(skb);
}
--
2.51.0
^ permalink raw reply related
* [PATCH net 0/5] rose: fix use-after-free and reference counting bugs
From: Bernard Pidoux @ 2026-04-26 14:43 UTC (permalink / raw)
To: netdev
Cc: linux-hams, linux-kernel, davem, edumazet, kuba, pabeni, horms,
Bernard Pidoux
This series fixes several bugs in the ROSE protocol loopback path and
state machines, all confirmed by KASAN on a live system.
A long-standing practical consequence of these bugs is that once the
rose module has been used to establish at least one connection, it can
no longer be unloaded cleanly: rmmod(8) hangs or causes a kernel crash
because outstanding references and running timers prevent the module
from completing its exit path. Patches 2 and 3 together fix this by
ensuring the loopback timer stops cleanly and all neighbour references
are released on module exit.
Patch 1 fixes a device reference leak in rose_loopback_timer(): dev_put()
was only called on the failure path of rose_rx_call_request(), leaking a
reference on every successful loopback CALL_REQUEST. It also removes a
dead check that can never be true for the loopback neighbour.
Patch 2 adds hold/put protection around the loopback timer body so that
rose_loopback_neigh cannot be freed while the callback is running.
Patch 3 fixes a race between the loopback timer and module removal by
switching rose_loopback_clear() to timer_delete_sync() and adding an
atomic loopback_stopping flag that prevents the timer from re-arming
itself after module exit has started.
Patch 4 clears rose->neighbour to NULL after every rose_neigh_put() call
in the ROSE state machines (rose_in.c), preventing use of a potentially
freed pointer by later code paths.
Patch 5 guards the rose_neigh_put() call in rose_timer_expiry() STATE_2
against a NULL pointer that can appear when a concurrent teardown path
(e.g. rose_kill_by_device()) has already cleared the neighbour pointer
between a timer re-arm and the next firing.
Bernard Pidoux (5):
rose: fix dev_put() leak in rose_loopback_timer()
rose: hold loopback neighbour reference across timer callback
rose: fix race between loopback timer and module removal
rose: clear neighbour pointer after rose_neigh_put() in state machines
rose: guard rose_neigh_put() against NULL in timer expiry
net/rose/rose_in.c | 6 +++++
net/rose/rose_loopback.c | 53 +++++++++++++++++++++++++++-------------
net/rose/rose_timer.c | 5 +++-
3 files changed, 46 insertions(+), 18 deletions(-)
--
2.51.0
^ permalink raw reply
* Re: [PATCH net 2/2] sctp: discard stale INIT after handshake completion
From: Xin Long @ 2026-04-26 14:37 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, kuba, Eric Dumazet, Paolo Abeni, Simon Horman,
Marcelo Ricardo Leitner, Florian Westphal, Yi Chen
In-Reply-To: <bea8a0dfcc56b9980cb914b54cffa9dd9948ba75.1775847557.git.lucien.xin@gmail.com>
> This is an AI-generated review of your patch from:
>
> https://sashiko.dev/#/patchset/cover.1775847557.git.lucien.xin%40gmail.com
>
> > @@ -1556,6 +1556,12 @@ static enum sctp_disposition sctp_sf_do_unexpected_init(
> > /* Tag the variable length parameters. */
> > chunk->param_hdr.v = skb_pull(chunk->skb, sizeof(struct sctp_inithdr));
> >
> > + if (asoc->state >= SCTP_STATE_ESTABLISHED) {
> > + /* Discard INIT matching peer vtag after handshake completion (stale INIT). */
> > + if (chunk->subh.init_hdr->init_tag == asoc->peer.i.init_tag)
> Does this code compare a network-byte-order value directly with a
> host-byte-order value?
> Looking at the structures, chunk->subh.init_hdr->init_tag is a __be32
> extracted directly from the packet, while asoc->peer.i.init_tag is a __u32.
> During handshake initialization, asoc->peer.i.init_tag is populated using
> ntohl().
> On little-endian architectures, will this comparison always fail and allow
> the stale INIT chunks to pass through instead of discarding them?
Will post v2 to fix this.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH 2/2] kernel/module: Decouple klp and ftrace from load_module
From: Song Chen @ 2026-04-26 14:26 UTC (permalink / raw)
To: Masami Hiramatsu (Google), Petr Mladek
Cc: Petr Pavlu, rafael, lenb, mturquette, sboyd, viresh.kumar, agk,
snitzer, mpatocka, bmarzins, song, yukuai, linan122, jason.wessel,
danielt, dianders, horms, davem, edumazet, kuba, pabeni, paulmck,
frederic, mcgrof, da.gomez, samitolvanen, atomlin, jpoimboe,
jikos, mbenes, joe.lawrence, rostedt, mark.rutland,
mathieu.desnoyers, linux-modules, linux-kernel,
linux-trace-kernel, linux-acpi, linux-clk, linux-pm,
live-patching, dm-devel, linux-raid, kgdb-bugreport, netdev
In-Reply-To: <20260420112707.aa3627ca9f975eeaf7d8ea0e@kernel.org>
Hi,
On 4/20/26 10:27, Masami Hiramatsu (Google) wrote:
> On Thu, 16 Apr 2026 16:49:32 +0200
> Petr Mladek <pmladek@suse.com> wrote:
>
>> On Thu 2026-04-16 13:18:30, Petr Pavlu wrote:
>>> On 4/15/26 8:43 AM, Song Chen wrote:
>>>> On 4/14/26 22:33, Petr Pavlu wrote:
>>>>> On 4/13/26 10:07 AM, chensong_2000@189.cn wrote:
>>>>>> diff --git a/include/linux/module.h b/include/linux/module.h
>>>>>> index 14f391b186c6..0bdd56f9defd 100644
>>>>>> --- a/include/linux/module.h
>>>>>> +++ b/include/linux/module.h
>>>>>> @@ -308,6 +308,14 @@ enum module_state {
>>>>>> MODULE_STATE_COMING, /* Full formed, running module_init. */
>>>>>> MODULE_STATE_GOING, /* Going away. */
>>>>>> MODULE_STATE_UNFORMED, /* Still setting it up. */
>>>>>> + MODULE_STATE_FORMED,
>>>>>
>>>>> I don't see a reason to add a new module state. Why is it necessary and
>>>>> how does it fit with the existing states?
>>>>>
>>>> because once notifier fails in state MODULE_STATE_UNFORMED (now only ftrace has someting to do in this state), notifier chain will roll back by calling blocking_notifier_call_chain_robust, i'm afraid MODULE_STATE_GOING is going to jeopardise the notifers which don't handle it appropriately, like:
>>>>
>>>> case MODULE_STATE_COMING:
>>>> kmalloc();
>>>> case MODULE_STATE_GOING:
>>>> kfree();
>>>
>>> My understanding is that the current module "state machine" operates as
>>> follows. Transitions marked with an asterisk (*) are announced via the
>>> module notifier.
>>>
>>> ---> UNFORMED --*> COMING --*> LIVE --*> GOING -.
>>> ^ | ^ |
>>> | '---------------------* |
>>> '---------------------------------------'
>>>
>>> The new code aims to replace the current ftrace_module_init() call in
>>> load_module(). To achieve this, it adds a notification for the UNFORMED
>>> state (only when loading a module) and introduces a new FORMED state for
>>> rollback. FORMED is purely a fake state because it never appears in
>>> module::state. The new structure is as follows:
>>>
>>> ,--*> (FORMED)
>>> |
>>> --*> UNFORMED --*> COMING --*> LIVE --*> GOING -.
>>> ^ | ^ |
>>> | '---------------------* |
>>> '---------------------------------------'
>>>
>>> I'm afraid this is quite complex and inconsistent. Unless it can be kept
>>> simple, we would be just replacing one special handling with a different
>>> complexity, which is not worth it.
>>
>>>>>
>>>>>> + if (err)
>>>>>> + goto ddebug_cleanup;
>>>>>> /* Finally it's fully formed, ready to start executing. */
>>>>>> err = complete_formation(mod, info);
>>>>>> - if (err)
>>>>>> + if (err) {
>>>>>> + blocking_notifier_call_chain_reverse(&module_notify_list,
>>>>>> + MODULE_STATE_FORMED, mod);
>>>>>> goto ddebug_cleanup;
>>>>>> + }
>>>>>> - err = prepare_coming_module(mod);
>>>>>> + err = prepare_module_state_transaction(mod,
>>>>>> + MODULE_STATE_COMING, MODULE_STATE_GOING);
>>>>>> if (err)
>>>>>> goto bug_cleanup;
>>>>>> @@ -3522,7 +3519,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
>>>>>> destroy_params(mod->kp, mod->num_kp);
>>>>>> blocking_notifier_call_chain(&module_notify_list,
>>>>>> MODULE_STATE_GOING, mod);
>>>>>
>>>>> My understanding is that all notifier chains for MODULE_STATE_GOING
>>>>> should be reversed.
>>>> yes, all, from lowest priority notifier to highest.
>>>> I will resend patch 1 which was failed due to my proxy setting.
>>>
>>> What I meant here is that the call:
>>>
>>> blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod);
>>>
>>> should be replaced with:
>>>
>>> blocking_notifier_call_chain_reverse(&module_notify_list, MODULE_STATE_GOING, mod);
>>>
>>>>
>>>>>
>>>>>> - klp_module_going(mod);
>>>>>> bug_cleanup:
>>>>>> mod->state = MODULE_STATE_GOING;
>>>>>> /* module_bug_cleanup needs module_mutex protection */
>>>>>
>>>>> The patch removes the klp_module_going() cleanup call in load_module().
>>>>> Similarly, the ftrace_release_mod() call under the ddebug_cleanup label
>>>>> should be removed and appropriately replaced with a cleanup via
>>>>> a notifier.
>>>>>
>>>> err = prepare_module_state_transaction(mod,
>>>> MODULE_STATE_UNFORMED, MODULE_STATE_FORMED);
>>>> if (err)
>>>> goto ddebug_cleanup;
>>>>
>>>> ftrace will be cleanup in blocking_notifier_call_chain_robust rolling back.
>>>>
>>>> err = prepare_module_state_transaction(mod,
>>>> MODULE_STATE_COMING, MODULE_STATE_GOING);
>>>>
>>>> each notifier including ftrace and klp will be cleanup in blocking_notifier_call_chain_robust rolling back.
>>>>
>>>> if all notifiers are successful in MODULE_STATE_COMING, they all will be clean up in
>>>> coming_cleanup:
>>>> mod->state = MODULE_STATE_GOING;
>>>> destroy_params(mod->kp, mod->num_kp);
>>>> blocking_notifier_call_chain(&module_notify_list,
>>>> MODULE_STATE_GOING, mod);
>>>>
>>>> if something wrong underneath.
>>>
>>> My point is that the patch leaves a call to ftrace_release_mod() in
>>> load_module(), which I expected to be handled via a notifier.
>>
>> I think that I have got it. The ftrace code needs two notifiers when
>> the module is being loaded and two when it is going.
>>
>> This is why Sond added the new state. But I think that we would
>> need two new states to call:
>>
>> + ftrace_module_init() in MODULE_STATE_UNFORMED
>> + ftrace_module_enable() in MODULE_STATE_FORMED
>>
>> and
>>
>> + ftrace_free_mem() in MODULE_STATE_PRE_GOING
>> + ftrace_free_mem() in MODULE_STATE_GOING
>>
>>
>> By using the ascii art:
>>
>> -*> UNFORMED -*> FORMED -> COMING -*> LIVE -*> PRE_GOING -*> GOING -.
>> | | | ^ ^ ^
>> | | '----------------' | |
>> | '--------------------------------------' |
>> '------------------------------------------------------'
>>
>>
>> But I think that this is not worth it.
>
> Agree.
>
> If this needs to be ordered so strictly, why we will use a "single"
> module notifier chain for this complex situation?
>
> I think the notifier call chain is just for notice a single signal,
> instead of sending several different signals, especially if there is
> any dependency among the callbacks.
>
> If notification callbacks need to be ordered, they are currently
> sorted by representing priority numerically, but this is quite
> fragile for updating. It has to look up other registered priorities
> and adjust the order among dependencies each time. For this reason,
> this mechanism is not suitable for global ordering. (It's like line
> numbers in BASIC.)
> It is probably only useful for representing dependencies between
> two components maintained by the same maintainer.
>
> I'm against a general-purpose system that makes everything modular.
> It unnecessarily complicates things. If there are processes that
> require strict ordering, especially processes that must be performed
> before each stage as part of the framework, they should be called
> directly from the framework, not via notification callbacks.
>
> This makes it simpler and more robust to maintain.
>
> Only the framework's end users should utilize notification callbacks.
>
> Thank you,
>
>
my motivation is to decouple ftrace and klp from module loader and make
blocking_notifier_chain more generic, but it doesn't become generic
completely. I understand your and Petr's comments and agree.
Thanks
Best regards
Song
>>
>> Best Regards,
>> Petr
>>
>
>
^ permalink raw reply
* [PATCH] net: lan966x: avoid unregistering netdev on register failure
From: Myeonghun Pak @ 2026-04-26 14:27 UTC (permalink / raw)
To: Horatiu Vultur, UNGLinuxDriver, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Ijae Kim
Cc: Myeonghun Pak, netdev, linux-kernel
lan966x_probe_port() stores the newly allocated net_device in the
port before calling register_netdev(). If register_netdev() fails,
the probe error path calls lan966x_cleanup_ports(), which sees
port->dev and calls unregister_netdev() for a device that was never
registered.
Destroy the phylink instance created for this port and clear port->dev
before returning the registration error, matching the existing guard
used by the common cleanup path.
Fixes: d28d6d2e37d1 ("net: lan966x: add port module support")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 47752d3fde..22c496f588 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -873,6 +873,9 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
err = register_netdev(dev);
if (err) {
dev_err(lan966x->dev, "register_netdev failed\n");
+ phylink_destroy(phylink);
+ port->phylink = NULL;
+ port->dev = NULL;
return err;
}
^ permalink raw reply related
* Re: [RFC PATCH 1/2] kernel/notifier: replace single-linked list with double-linked list for reverse traversal
From: Song Chen @ 2026-04-26 14:14 UTC (permalink / raw)
To: Masami Hiramatsu (Google)
Cc: rafael, lenb, mturquette, sboyd, viresh.kumar, agk, snitzer,
mpatocka, bmarzins, song, yukuai, linan122, jason.wessel, danielt,
dianders, horms, davem, edumazet, kuba, pabeni, paulmck, frederic,
mcgrof, petr.pavlu, da.gomez, samitolvanen, atomlin, jpoimboe,
jikos, mbenes, pmladek, joe.lawrence, rostedt, mark.rutland,
mathieu.desnoyers, linux-modules, linux-kernel,
linux-trace-kernel, linux-acpi, linux-clk, linux-pm,
live-patching, dm-devel, linux-raid, kgdb-bugreport, netdev
In-Reply-To: <20260420144429.57b45f2beece690bceea96ec@kernel.org>
Hi Hiramatsu san,
On 4/20/26 13:44, Masami Hiramatsu (Google) wrote:
> Hi Song,
>
> On Wed, 15 Apr 2026 15:01:37 +0800
> chensong_2000@189.cn wrote:
>
>> From: Song Chen <chensong_2000@189.cn>
>>
>> The current notifier chain implementation uses a single-linked list
>> (struct notifier_block *next), which only supports forward traversal
>> in priority order. This makes it difficult to handle cleanup/teardown
>> scenarios that require notifiers to be called in reverse priority order.
>
> What about introducing a new notification callback API that allows you
> to describe dependencies between callback functions?
>
> For example, when registering a callback, you could register a string
> as an ID and specify whether to call it before or after that ID,
> or you could register a comparison function that is called when adding
> to a list. (I prefer @name and @depends fields so that it can be easily
> maintained.)
>
> This would allow for better dependency building when adding to the list.
>
Is the new notification callback API going to replace
blocking_notifier_chain in module loader? or an expansion inside
blocking_notifier_chain but introducing less complexity?
>>
>> A concrete example is the ordering dependency between ftrace and
>> livepatch during module load/unload. see the detail here [1].
>
> If this only concerns notification callback issues with the ftrace
> and livepatch modules, it's far more robust to simply call the
> necessary processing directly when the modules load and unload,
> rather than registering notification callbacks externally.
>
> There are fprobe, kprobe and its trace-events, all of them are using
> ftrace as its fundation layer. In this case, I always needs to
> consider callback order when a module is unloaded.
>
> If ftrace is working as a part of module callbacks, it will conflict
> with fprobe/kprobe module callback. Of course we can reorder it with
> modifying its priority. But this is ugly, because when we introduce
> a new other feature which depends on another layer, we need to
> reorder the callback's priority number on the list.
>
> Based on the above, I don't think this can be resolved simply by
> changing the list of notification callbacks to a bidirectional list.
>
> Thank you,
>
understood, many thanks for your proposal, i will think about it.
best regards,
Song
^ permalink raw reply
* [PATCH DRAFT] net: cirrus: ep93xx: fix probe error unwind
From: 박명훈 @ 2026-04-26 14:10 UTC (permalink / raw)
To: Hartley Sweeten, Andrew Lunn, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Ijae Kim
Cc: netdev, linux-kernel, Myeonghun Pak
From: Myeonghun Pak <mhun512@gmail.com>
ep93xx_eth_probe() uses ep93xx_eth_remove() as common error unwind
after setting driver data. When register_netdev() fails, this calls
unregister_netdev() for a net_device that was never registered. The
net core treats that as a driver bug and emits WARN_ON(1).
The shared remove path also fails to unmap the register mapping on
earlier allocation and resource-reservation failures, because
ep->base_addr is only assigned after request_mem_region() succeeds.
Unwind probe failures directly and publish driver data only after the
netdev is registered. This keeps .remove() for successful probes only,
releases the memory region on late failures, frees the net_device, and
unmaps the registers.
Fixes: 1d22e05df818 ("[PATCH] Cirrus Logic ep93xx ethernet driver")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
diff --git a/drivers/net/ethernet/cirrus/ep93xx_eth.c b/drivers/net/ethernet/cirrus/ep93xx_eth.c
index a4972457ed..4e98b76ec7 100644
--- a/drivers/net/ethernet/cirrus/ep93xx_eth.c
+++ b/drivers/net/ethernet/cirrus/ep93xx_eth.c
@@ -800,7 +800,7 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
dev = alloc_etherdev(sizeof(struct ep93xx_priv));
if (dev == NULL) {
err = -ENOMEM;
- goto err_out;
+ goto err_iounmap;
}
memcpy_fromio(addr, base_addr + 0x50, ETH_ALEN);
@@ -814,14 +814,12 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
SET_NETDEV_DEV(dev, &pdev->dev);
netif_napi_add(dev, &ep->napi, ep93xx_poll);
- platform_set_drvdata(pdev, dev);
-
ep->res = request_mem_region(mem->start, resource_size(mem),
dev_name(&pdev->dev));
if (ep->res == NULL) {
dev_err(&pdev->dev, "Could not reserve memory region\n");
err = -ENOMEM;
- goto err_out;
+ goto err_free_netdev;
}
ep->base_addr = base_addr;
@@ -841,16 +839,22 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
err = register_netdev(dev);
if (err) {
dev_err(&pdev->dev, "Failed to register netdev\n");
- goto err_out;
+ goto err_release_mem;
}
+ platform_set_drvdata(pdev, dev);
+
printk(KERN_INFO "%s: ep93xx on-chip ethernet, IRQ %d, %pM\n",
dev->name, ep->irq, dev->dev_addr);
return 0;
-err_out:
- ep93xx_eth_remove(pdev);
+err_release_mem:
+ release_mem_region(mem->start, resource_size(mem));
+err_free_netdev:
+ free_netdev(dev);
+err_iounmap:
+ iounmap(base_addr);
return err;
}
^ permalink raw reply related
* Re: [RFC PATCH 1/2] kernel/notifier: replace single-linked list with double-linked list for reverse traversal
From: Song Chen @ 2026-04-26 13:56 UTC (permalink / raw)
To: Petr Mladek, Masami Hiramatsu
Cc: chensong_2000, rafael, lenb, mturquette, sboyd, viresh.kumar, agk,
snitzer, mpatocka, bmarzins, song, yukuai, linan122, jason.wessel,
danielt, dianders, horms, davem, edumazet, kuba, pabeni, paulmck,
frederic, mcgrof, petr.pavlu, da.gomez, samitolvanen, atomlin,
jpoimboe, jikos, mbenes, joe.lawrence, rostedt, mark.rutland,
mathieu.desnoyers, linux-modules, linux-kernel,
linux-trace-kernel, linux-acpi, linux-clk, linux-pm,
live-patching, dm-devel, linux-raid, kgdb-bugreport, netdev
In-Reply-To: <aec90caYZDHDAHgw@pathway.suse.cz>
Hi,
On 4/21/26 17:05, Petr Mladek wrote:
> On Mon 2026-04-20 14:44:29, Masami Hiramatsu wrote:
>> Hi Song,
>>
>> On Wed, 15 Apr 2026 15:01:37 +0800
>> chensong_2000@189.cn wrote:
>>
>>> From: Song Chen <chensong_2000@189.cn>
>>>
>>> The current notifier chain implementation uses a single-linked list
>>> (struct notifier_block *next), which only supports forward traversal
>>> in priority order. This makes it difficult to handle cleanup/teardown
>>> scenarios that require notifiers to be called in reverse priority order.
>>
>> What about introducing a new notification callback API that allows you
>> to describe dependencies between callback functions?
>>
>> For example, when registering a callback, you could register a string
>> as an ID and specify whether to call it before or after that ID,
>> or you could register a comparison function that is called when adding
>> to a list. (I prefer @name and @depends fields so that it can be easily
>> maintained.)
>
> This looks too complex. It would make sense only
> when this API has more users.
>
> Also this won't be enough for the ftrace/livepatch callbacks.
> They need to be ordered against against each other. But they
> also need to be called before/after all other callbacks.
> For example, when the module is loaded:
>
> + 1st frace
> + 2nd livepatch
> + then other notifiers
>
> See the commit c1bf08ac26e92122 ("ftrace: Be first to run code
> modification on modules").
>
>> This would allow for better dependency building when adding to the list.
>
>>>
>>> A concrete example is the ordering dependency between ftrace and
>>> livepatch during module load/unload. see the detail here [1].
>>
>> If this only concerns notification callback issues with the ftrace
>> and livepatch modules, it's far more robust to simply call the
>> necessary processing directly when the modules load and unload,
>> rather than registering notification callbacks externally.
>>
>> There are fprobe, kprobe and its trace-events, all of them are using
>> ftrace as its fundation layer. In this case, I always needs to
>> consider callback order when a module is unloaded.
>>
>> If ftrace is working as a part of module callbacks, it will conflict
>> with fprobe/kprobe module callback. Of course we can reorder it with
>> modifying its priority. But this is ugly, because when we introduce
>> a new other feature which depends on another layer, we need to
>> reorder the callback's priority number on the list.
>>
>> Based on the above, I don't think this can be resolved simply by
>> changing the list of notification callbacks to a bidirectional list.
>
> I agree. I would keep it as is (hardcoded).
>
> Best Regards,
> Petr
>
Thanks for the feedback, the necessity doesn't convincing enough. I will
try the proposal from Masami Hiramatsu.
Best regards,
Song
^ permalink raw reply
* [PATCH net] bridge: mcast: Fix a false positive lockdep splat
From: Ido Schimmel @ 2026-04-26 13:34 UTC (permalink / raw)
To: netdev, bridge
Cc: davem, kuba, pabeni, edumazet, razor, horms, herbert,
linus.luessing, Ido Schimmel
Connecting two bridges on the same system [1] can result in a lockdep
splat [2].
The report is a false positive. Multicast queries are built and
transmitted under the bridge multicast lock. When the outgoing port of
one bridge is configured on top of another bridge, the transmit path
re-enters bridge code and acquires the other bridge's multicast lock in
order to snoop the query. Both lock instances share a single lockdep
class, so lockdep flags the nested acquisition as an AA deadlock.
Giving each bridge its own lock class will not solve the problem: the
reverse topology would produce an ABBA splat with the same pair of
classes. It also consumes a lockdep key per bridge.
Instead, fix the problem by deferring the transmission of the queries to
a workqueue. Build the skb and update querier state under the lock as
before, then enqueue the skb on a per multicast context queue and
schedule the work.
Flush the work when the multicast context is de-initialized. At this
stage the work cannot be requeued. There is no need to take a reference
on skb->dev since the work cannot outlive the bridge or the bridge port.
Use the high priority workqueue to reduce the delay between the enqueue
time and the transmission time. With default settings (i.e., querier
interval - 255 seconds, query interval - 125 seconds) the extra delay
should not be a problem.
[1]
ip link add name br1 up type bridge mcast_snooping 1 mcast_querier 1
ip link add name br0 up type bridge mcast_snooping 1 mcast_querier 1
ip link add link br0 name br0.10 up master br1 type vlan id 10
[2]
============================================
WARNING: possible recursive locking detected
7.0.0-virtme-gb50c64a58a90 #1 Not tainted
--------------------------------------------
ip/339 is trying to acquire lock:
ffff888104f0b480 (&br->multicast_lock){+.-.}-{3:3}, at: br_ip6_multicast_query (net/bridge/br_multicast.c:3584)
but task is already holding lock:
ffff888104f03480 (&br->multicast_lock){+.-.}-{3:3}, at: br_multicast_port_query_expired (net/bridge/br_multicast.c:1904)
[...]
Call Trace:
[...]
br_ip6_multicast_query (net/bridge/br_multicast.c:3584)
br_multicast_ipv6_rcv (net/bridge/br_multicast.c:3988)
br_dev_xmit (net/bridge/br_device.c:98 (discriminator 1))
dev_hard_start_xmit (./include/linux/netdevice.h:5343 ./include/linux/netdevice.h:5352 net/core/dev.c:3888 net/core/dev.c:3904)
__dev_queue_xmit (./include/linux/netdevice.h:3619 net/core/dev.c:4871)
vlan_dev_hard_start_xmit (net/8021q/vlan_dev.c:131 (discriminator 1))
dev_hard_start_xmit (./include/linux/netdevice.h:5343 ./include/linux/netdevice.h:5352 net/core/dev.c:3888 net/core/dev.c:3904)
__dev_queue_xmit (./include/linux/netdevice.h:3619 net/core/dev.c:4871)
br_dev_queue_push_xmit (net/bridge/br_forward.c:60)
__br_multicast_send_query (net/bridge/br_multicast.c:1811 (discriminator 1))
br_multicast_send_query (net/bridge/br_multicast.c:1889)
br_multicast_port_query_expired (./include/linux/spinlock.h:390 net/bridge/br_multicast.c:1914)
call_timer_fn (./arch/x86/include/asm/jump_label.h:37 ./include/trace/events/timer.h:127 kernel/time/timer.c:1749)
[...]
Fixes: eb1d16414339 ("bridge: Add core IGMP snooping support")
Reported-by: syzbot+d7b7f1412c02134efa6d@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/000000000000c4c9d405f2643e01@google.com/
Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
net/bridge/br_multicast.c | 39 +++++++++++++++++++++++++++++++++++----
net/bridge/br_private.h | 4 ++++
2 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 881d866d687a..252c46977ed5 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1776,6 +1776,28 @@ static void br_multicast_select_own_querier(struct net_bridge_mcast *brmctx,
#endif
}
+static void br_multicast_port_query_queue_work(struct work_struct *work)
+{
+ struct net_bridge_mcast_port *pmctx;
+ struct sk_buff *skb;
+
+ pmctx = container_of(work, struct net_bridge_mcast_port,
+ query_queue_work);
+ while ((skb = skb_dequeue(&pmctx->query_queue)))
+ NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_OUT, dev_net(skb->dev),
+ NULL, skb, NULL, skb->dev, br_dev_queue_push_xmit);
+}
+
+static void br_multicast_query_queue_work(struct work_struct *work)
+{
+ struct net_bridge_mcast *brmctx;
+ struct sk_buff *skb;
+
+ brmctx = container_of(work, struct net_bridge_mcast, query_queue_work);
+ while ((skb = skb_dequeue(&brmctx->query_queue)))
+ netif_rx(skb);
+}
+
static void __br_multicast_send_query(struct net_bridge_mcast *brmctx,
struct net_bridge_mcast_port *pmctx,
struct net_bridge_port_group *pg,
@@ -1804,9 +1826,8 @@ static void __br_multicast_send_query(struct net_bridge_mcast *brmctx,
skb->dev = pmctx->port->dev;
br_multicast_count(brmctx->br, pmctx->port, skb, igmp_type,
BR_MCAST_DIR_TX);
- NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_OUT,
- dev_net(pmctx->port->dev), NULL, skb, NULL, skb->dev,
- br_dev_queue_push_xmit);
+ skb_queue_tail(&pmctx->query_queue, skb);
+ queue_work(system_highpri_wq, &pmctx->query_queue_work);
if (over_lmqt && with_srcs && sflag) {
over_lmqt = false;
@@ -1816,7 +1837,8 @@ static void __br_multicast_send_query(struct net_bridge_mcast *brmctx,
br_multicast_select_own_querier(brmctx, group, skb);
br_multicast_count(brmctx->br, NULL, skb, igmp_type,
BR_MCAST_DIR_RX);
- netif_rx(skb);
+ skb_queue_tail(&brmctx->query_queue, skb);
+ queue_work(system_highpri_wq, &brmctx->query_queue_work);
}
}
@@ -1999,6 +2021,10 @@ void br_multicast_port_ctx_init(struct net_bridge_port *port,
pmctx->port = port;
pmctx->vlan = vlan;
pmctx->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
+
+ skb_queue_head_init(&pmctx->query_queue);
+ INIT_WORK(&pmctx->query_queue_work, br_multicast_port_query_queue_work);
+
timer_setup(&pmctx->ip4_mc_router_timer,
br_ip4_multicast_router_expired, 0);
timer_setup(&pmctx->ip4_own_query.timer,
@@ -2038,6 +2064,7 @@ void br_multicast_port_ctx_deinit(struct net_bridge_mcast_port *pmctx)
del |= br_ip4_multicast_rport_del(pmctx);
br_multicast_rport_del_notify(pmctx, del);
spin_unlock_bh(&br->multicast_lock);
+ flush_work(&pmctx->query_queue_work);
}
int br_multicast_add_port(struct net_bridge_port *port)
@@ -4111,6 +4138,9 @@ void br_multicast_ctx_init(struct net_bridge *br,
seqcount_spinlock_init(&brmctx->ip6_querier.seq, &br->multicast_lock);
#endif
+ skb_queue_head_init(&brmctx->query_queue);
+ INIT_WORK(&brmctx->query_queue_work, br_multicast_query_queue_work);
+
timer_setup(&brmctx->ip4_mc_router_timer,
br_ip4_multicast_local_router_expired, 0);
timer_setup(&brmctx->ip4_other_query.timer,
@@ -4134,6 +4164,7 @@ void br_multicast_ctx_init(struct net_bridge *br,
void br_multicast_ctx_deinit(struct net_bridge_mcast *brmctx)
{
__br_multicast_stop(brmctx);
+ flush_work(&brmctx->query_queue_work);
}
void br_multicast_init(struct net_bridge *br)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 361a9b84451e..2695b9128705 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -131,6 +131,8 @@ struct net_bridge_mcast_port {
unsigned char multicast_router;
u32 mdb_n_entries;
u32 mdb_max_entries;
+ struct sk_buff_head query_queue;
+ struct work_struct query_queue_work;
#endif /* CONFIG_BRIDGE_IGMP_SNOOPING */
};
@@ -167,6 +169,8 @@ struct net_bridge_mcast {
struct bridge_mcast_own_query ip6_own_query;
struct bridge_mcast_querier ip6_querier;
#endif /* IS_ENABLED(CONFIG_IPV6) */
+ struct sk_buff_head query_queue;
+ struct work_struct query_queue_work;
#endif /* CONFIG_BRIDGE_IGMP_SNOOPING */
};
--
2.53.0
^ permalink raw reply related
* Re: [PATCH net V3 4/4] net/mlx5e: SD, Fix race condition in secondary device probe/remove
From: Shay Drori @ 2026-04-26 13:26 UTC (permalink / raw)
To: Tariq Toukan, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Lunn, David S. Miller
Cc: Saeed Mahameed, Mark Bloch, Leon Romanovsky, Simon Horman,
Kees Cook, Patrisious Haddad, Parav Pandit, Gal Pressman, netdev,
linux-rdma, linux-kernel, Dragos Tatulea
In-Reply-To: <20260423123104.201552-5-tariqt@nvidia.com>
On 23/04/2026 15:31, Tariq Toukan wrote:
> From: Shay Drory <shayd@nvidia.com>
>
> When utilizing Socket-Direct single netdev functionality the driver
> resolves the actual auxiliary device using mlx5_sd_get_adev(). However,
> the current implementation returns the primary ETH auxiliary device
> without holding the device lock, leading to a potential race condition
> where the ETH device could be unbound or removed concurrently during
> probe, suspend, resume, or remove operations.[1]
>
> Fix this by introducing mlx5_sd_put_adev() and updating
> mlx5_sd_get_adev() so that secondaries devices would acquire the device
> lock of the returned auxiliary device. After the lock is acquired, a
> second devcom check is needed[2].
> In addition, update The callers to pair the get operation with the new
> put operation, ensuring the lock is held while the auxiliary device is
> being operated on and released afterwards.
>
> The "primary" designation is determined once in sd_register(). It's set
> before devcom is marked ready, and it never changes after that.
> In Addition, The primary path never locks a secondary: When the primary
> device invoke mlx5_sd_get_adev(), it sees dev == primary and returns.
> no additional lock is taken.
> Therefore lock ordering is always: secondary_lock -> primary_lock. The
> reverse never happens, so ABBA deadlock is impossible.
>
> [1]
> for example:
> BUG: kernel NULL pointer dereference, address: 0000000000000370
> PGD 0 P4D 0
> Oops: Oops: 0000 [#1] SMP
> CPU: 4 UID: 0 PID: 3945 Comm: bash Not tainted 6.19.0-rc3+ #1 NONE
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> RIP: 0010:mlx5e_dcbnl_dscp_app+0x23/0x100 [mlx5_core]
> Call Trace:
> <TASK>
> mlx5e_remove+0x82/0x12a [mlx5_core]
> device_release_driver_internal+0x194/0x1f0
> bus_remove_device+0xc6/0x140
> device_del+0x159/0x3c0
> ? devl_param_driverinit_value_get+0x29/0x80
> mlx5_rescan_drivers_locked+0x92/0x160 [mlx5_core]
> mlx5_unregister_device+0x34/0x50 [mlx5_core]
> mlx5_uninit_one+0x43/0xb0 [mlx5_core]
> remove_one+0x4e/0xc0 [mlx5_core]
> pci_device_remove+0x39/0xa0
> device_release_driver_internal+0x194/0x1f0
> unbind_store+0x99/0xa0
> kernfs_fop_write_iter+0x12e/0x1e0
> vfs_write+0x215/0x3d0
> ksys_write+0x5f/0xd0
> do_syscall_64+0x55/0xe90
> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
> [2]
> CPU0 (primary) CPU1 (secondary)
> ==========================================================================
> mlx5e_remove() (device_lock held)
> mlx5e_remove() (2nd device_lock held)
> mlx5_sd_get_adev()
> mlx5_devcom_comp_is_ready() => true
> device_lock(primary)
> mlx5_sd_get_adev() ==> ret adev
> _mlx5e_remove()
> mlx5_sd_cleanup()
> // mlx5e_remove finished
> // releasing device_lock
> //need another check here...
> mlx5_devcom_comp_is_ready() => false
>
> Fixes: 381978d28317 ("net/mlx5e: Create single netdev per SD group")
> Signed-off-by: Shay Drory <shayd@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_main.c | 10 ++++++++++
> .../net/ethernet/mellanox/mlx5/core/lib/sd.c | 17 +++++++++++++++++
> .../net/ethernet/mellanox/mlx5/core/lib/sd.h | 2 ++
> 3 files changed, 29 insertions(+)
I went over all Sashiko comments on this patch and they are false
positive
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 9c340ad2fe09..c4cb5369f0a0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -6778,11 +6778,14 @@ static int mlx5e_resume(struct auxiliary_device *adev)
> err = _mlx5e_resume(actual_adev);
> if (err)
> goto sd_cleanup;
> + mlx5_sd_put_adev(actual_adev, adev);
> }
> return 0;
>
> sd_cleanup:
> mlx5_sd_cleanup(mdev);
> + if (actual_adev)
> + mlx5_sd_put_adev(actual_adev, adev);
> return err;
> }
>
> @@ -6822,6 +6825,8 @@ static int mlx5e_suspend(struct auxiliary_device *adev, pm_message_t state)
> err = _mlx5e_suspend(actual_adev, false);
>
> mlx5_sd_cleanup(mdev);
> + if (actual_adev)
> + mlx5_sd_put_adev(actual_adev, adev);
> return err;
> }
>
> @@ -6923,11 +6928,14 @@ static int mlx5e_probe(struct auxiliary_device *adev,
> err = _mlx5e_probe(actual_adev);
> if (err)
> goto sd_cleanup;
> + mlx5_sd_put_adev(actual_adev, adev);
> }
> return 0;
>
> sd_cleanup:
> mlx5_sd_cleanup(mdev);
> + if (actual_adev)
> + mlx5_sd_put_adev(actual_adev, adev);
> return err;
> }
>
> @@ -6980,6 +6988,8 @@ static void mlx5e_remove(struct auxiliary_device *adev)
> _mlx5e_remove(actual_adev);
>
> mlx5_sd_cleanup(mdev);
> + if (actual_adev)
> + mlx5_sd_put_adev(actual_adev, adev);
> }
>
> static const struct auxiliary_device_id mlx5e_id_table[] = {
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> index 897b0d81b27d..f7b226823ab4 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> @@ -538,6 +538,10 @@ void mlx5_sd_cleanup(struct mlx5_core_dev *dev)
> sd_cleanup(dev);
> }
>
> +/* Cannot take devcom lock as a gate for device lock. ABBA deadlock:
> + * primary: actual_adev_lock -> SD devcom comp lock
> + * secondary: SD devcom comp lock -> actual_adev_lock
> + */
> struct auxiliary_device *mlx5_sd_get_adev(struct mlx5_core_dev *dev,
> struct auxiliary_device *adev,
> int idx)
> @@ -555,5 +559,18 @@ struct auxiliary_device *mlx5_sd_get_adev(struct mlx5_core_dev *dev,
> if (dev == primary)
> return adev;
>
> + device_lock(&primary->priv.adev[idx]->adev.dev);
> + /* In case primary finish removing its adev */
> + if (!mlx5_devcom_comp_is_ready(sd->devcom)) {
> + device_unlock(&primary->priv.adev[idx]->adev.dev);
> + return NULL;
> + }
> return &primary->priv.adev[idx]->adev;
> }
> +
> +void mlx5_sd_put_adev(struct auxiliary_device *actual_adev,
> + struct auxiliary_device *adev)
> +{
> + if (actual_adev != adev)
> + device_unlock(&actual_adev->dev);
> +}
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.h b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.h
> index 137efaf9aabc..9bfd5b9756b5 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.h
> @@ -15,6 +15,8 @@ struct mlx5_core_dev *mlx5_sd_ch_ix_get_dev(struct mlx5_core_dev *primary, int c
> struct auxiliary_device *mlx5_sd_get_adev(struct mlx5_core_dev *dev,
> struct auxiliary_device *adev,
> int idx);
> +void mlx5_sd_put_adev(struct auxiliary_device *actual_adev,
> + struct auxiliary_device *adev);
>
> int mlx5_sd_init(struct mlx5_core_dev *dev);
> void mlx5_sd_cleanup(struct mlx5_core_dev *dev);
^ permalink raw reply
* Re: [PATCH net v2] ipv6: Implement limits on extension header parsing
From: Ido Schimmel @ 2026-04-26 13:17 UTC (permalink / raw)
To: Daniel Borkmann, tom, justin.iurman
Cc: Justin Iurman, kuba, edumazet, dsahern, tom,
willemdebruijn.kernel, pabeni, netdev
In-Reply-To: <90c7de29-2641-413d-9d5f-5eb323cf875c@iogearbox.net>
On Sun, Apr 26, 2026 at 12:38:31PM +0200, Daniel Borkmann wrote:
> On 4/25/26 12:19 PM, Justin Iurman wrote:
> > I've given it a lot of thought. I came to the conclusion that we
> > should use a hard-coded value here as well (just like we did for
> > 076b8cad77aa, with the same logic), not a sysctl. IMO, the main
> > reason is that it provides as is a suitable security fix to be
> > backported, i.e., the max value is the max number of EHs allowed by
> > RFC 8200, Section 4.1. Also, we remain consistent with
> > draft-iurman-6man-eh-occurrences (I think Tom is about to send a
> > revision of the series soon for net-next). What this series does is
> > not only enforcing ordering, but also verifying the specific number
> > of occurrences for each type of Extension Header. Which is totally
> > compatible with what this patch does, i.e., limiting the total
> > number of Extension Headers (regardless of their types) to 8. I
> > guess what I'm trying to say is that it seems like a good
> > plan/compromise and that the aforementioned series would build
> > perfectly on top of this fix.
>
> Initially, I had a hard-coded constant (when it was still 32), but Eric's comment
> was to rather go with a sysctl, such that if someone unexpectedly complains, then
> there is still a chance for that person to fix it up via sysctl without having to
> rebuild the kernel. I'm okay either way, but presumably given we're now being more
> "aggressive" into lowering the default to 8 rather than 32 then having such a fall-
> back is probably better.
I also think that 32 without a sysctl knob is fine (just so that we have
some upper bound), but if we go with a sysctl then let's make sure that
it's compatible with Tom's series [1] (I assume he is going to send a
new version).
AFAICT it's possible to create conflicting configuration with both
sysctls (e.g., "enforce_ext_hdr_order" is set to 1 and
"max_ext_hdrs_number" configured to less than 8). The documentation
should make the relation between both sysctls clear to users. It can
also mention that "max_ext_hdrs_number" might be useful when users are
forced to turn "enforce_ext_hdr_order" off when dealing with hosts that
send extension headers in an unexpected order. That way, they still have
an upper bound on the maximum number of extension headers.
[1] https://lore.kernel.org/netdev/20260314175124.47010-1-tom@herbertland.com/
^ permalink raw reply
* Re: [PATCH net V2 1/2] net/mlx5e: psp: Fix invalid access on PSP dev registration fail
From: Willem de Bruijn @ 2026-04-26 13:17 UTC (permalink / raw)
To: Tariq Toukan, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Lunn, David S. Miller
Cc: Boris Pismenny, Saeed Mahameed, Leon Romanovsky, Tariq Toukan,
Mark Bloch, Daniel Zahka, Willem de Bruijn, Cosmin Ratiu,
Raed Salem, Rahul Rameshbabu, Dragos Tatulea, Kees Cook, netdev,
linux-rdma, linux-kernel, Gal Pressman
In-Reply-To: <20260426083819.208937-2-tariqt@nvidia.com>
Tariq Toukan wrote:
> From: Cosmin Ratiu <cratiu@nvidia.com>
>
> priv->psp->psp is initialized with the PSP device as returned by
> psp_dev_create(). This could also return an error, in which case a
> future psp_dev_unregister() will result in unpleasantness.
>
> Avoid that by using a local variable and only saving the PSP device when
> registration succeeds.
> Also apply some light refactoring of the functions managing the PSP
> device in order to make them more readable/safe.
This is generally discouraged as it obfuscates the fix.
That said, the fix on its own makes sense.
> In case psp_dev_create() fails, priv->psp and steering structs are left
> in place, but they will be inert. The unchecked access of priv->psp in
> mlx5e_psp_offload_handle_rx_skb() won't happen because without a PSP
> device, there can be no SAs added and therefore no packets will be
> successfully decrypted and be handed off to the SW handler.
>
> Fixes: 89ee2d92f66c ("net/mlx5e: Support PSP offload functionality")
> Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
^ permalink raw reply
* Re: [PATCH net] net: mana: hardening: Validate SHM offset from BAR0 register to prevent crash due to alignment fault
From: Jason Gunthorpe @ 2026-04-26 13:15 UTC (permalink / raw)
To: Dipayaan Roy
Cc: kys, haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet,
kuba, pabeni, leon, longli, kotaranov, horms, shradhagupta,
ssengar, ernis, shirazsaleem, linux-hyperv, netdev, linux-kernel,
linux-rdma, stephen, jacob.e.keller, dipayanroy, leitao, kees,
john.fastabend, hawk, bpf, daniel, ast, sdf, yury.norov
In-Reply-To: <aepF3NwyANeklkfD@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
On Thu, Apr 23, 2026 at 09:16:28AM -0700, Dipayaan Roy wrote:
> During Function Level Reset recovery, the MANA driver reads
> hardware BAR0 registers that may temporarily contain garbage values.
> The SHM (Shared Memory) offset read from GDMA_REG_SHM_OFFSET is used
> to compute gc->shm_base, which is later dereferenced via readl() in
> mana_smc_poll_register(). If the hardware returns an unaligned or
> out-of-range value, the driver must not blindly use it, as this would
> propagate the hardware error into a kernel crash.
It is not what we are calling "hardening" if you are hitting actual
crashes in actual real systems.
"hardening" is the driver defending against actively malicious
hardware, operating in ways that will never be seen in real systems,
attempting to compromise the kernel.
Drivers working around real world broken/buggy/malfunctioning HW is
just entirely normal stuff.
> @@ -73,10 +74,25 @@ static int mana_gd_init_pf_regs(struct pci_dev *pdev)
> gc->phys_db_page_base = gc->bar0_pa + gc->db_page_off;
>
> sriov_base_off = mana_gd_r64(gc, GDMA_SRIOV_REG_CFG_BASE_OFF);
> + if (sriov_base_off >= gc->bar0_size ||
> + !IS_ALIGNED(sriov_base_off, sizeof(u32))) {
> + dev_err(gc->dev,
> + "SRIOV base offset 0x%llx out of range or unaligned (BAR0 size 0x%llx)\n",
> + sriov_base_off, (u64)gc->bar0_size);
> + return -EPROTO;
> + }
.. and if it is entirely normal and something that happens is EPROTO
really the right way to deal with this race, or should the driver be
looping somehow until the device stabilizes??
Jason
^ permalink raw reply
* Re: [PATCH net v5 1/2] net/sched: taprio: fix NULL pointer dereference in class dump
From: Weiming Shi @ 2026-04-26 13:01 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: vinicius.gomes, jiri, davem, edumazet, kuba, pabeni, horms,
vladimir.oltean, shuah, xmei5, netdev, linux-kselftest
In-Reply-To: <CAM0EoMka18Lsu94wTFZU3ntXP9Z3vNy0Get+=THPdsCafcoPnw@mail.gmail.com>
On 26-04-26 06:59, Jamal Hadi Salim wrote:
> On Wed, Apr 22, 2026 at 12:20 PM Weiming Shi <bestswngs@gmail.com> wrote:
> >
> > When a TAPRIO child qdisc is deleted via RTM_DELQDISC, taprio_graft()
> > is called with new == NULL and stores NULL into q->qdiscs[cl - 1].
> > Subsequent RTM_GETTCLASS dump operations walk all classes via
> > taprio_walk() and call taprio_dump_class(), which calls taprio_leaf()
> > returning the NULL pointer, then dereferences it to read child->handle,
> > causing a kernel NULL pointer dereference.
> >
> > The bug is reachable with namespace-scoped CAP_NET_ADMIN on any kernel
> > with CONFIG_NET_SCH_TAPRIO enabled. On systems with unprivileged user
> > namespaces enabled, an unprivileged local user can trigger a kernel
> > panic by creating a taprio qdisc inside a new network namespace,
> > grafting an explicit child qdisc, deleting it, and requesting a class
> > dump. The RTM_GETTCLASS dump itself requires no capability.
> >
> > Oops: general protection fault, probably for non-canonical address 0xdffffc0000000007: 0000 [#1] SMP KASAN NOPTI
> > KASAN: null-ptr-deref in range [0x0000000000000038-0x000000000000003f]
> > RIP: 0010:taprio_dump_class (net/sched/sch_taprio.c:2478)
> > Call Trace:
> > <TASK>
> > tc_fill_tclass (net/sched/sch_api.c:1966)
> > qdisc_class_dump (net/sched/sch_api.c:2326)
> > taprio_walk (net/sched/sch_taprio.c:2514)
> > tc_dump_tclass_qdisc (net/sched/sch_api.c:2352)
> > tc_dump_tclass_root (net/sched/sch_api.c:2370)
> > tc_dump_tclass (net/sched/sch_api.c:2431)
> > rtnl_dumpit (net/core/rtnetlink.c:6864)
> > netlink_dump (net/netlink/af_netlink.c:2325)
> > rtnetlink_rcv_msg (net/core/rtnetlink.c:6959)
> > netlink_rcv_skb (net/netlink/af_netlink.c:2550)
> > </TASK>
> >
> > Fix this by substituting &noop_qdisc when new is NULL in
> > taprio_graft(), a common pattern used by other qdiscs (e.g.,
> > multiq_graft()) to ensure the q->qdiscs[] slots are never NULL.
> > This makes control-plane dump paths safe without requiring individual
> > NULL checks.
> >
> > Since the data-plane paths (taprio_enqueue and taprio_dequeue_from_txq)
> > previously had explicit NULL guards that would drop/skip the packet
> > cleanly, update those checks to test for &noop_qdisc instead. Without
> > this, packets would reach taprio_enqueue_one() which increments the root
> > qdisc's qlen and backlog before calling the child's enqueue; noop_qdisc
> > drops the packet but those counters are never rolled back, permanently
> > inflating the root qdisc's statistics.
> >
> > After this change *old can be a valid qdisc, NULL, or &noop_qdisc.
> > Only call qdisc_put(*old) in the first case to avoid decreasing
> > noop_qdisc's refcount, which was never increased.
> >
> > Fixes: 665338b2a7a0 ("net/sched: taprio: dump class stats for the actual q->qdiscs[]")
> > Reported-by: Xiang Mei <xmei5@asu.edu>
> > Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> > Assisted-by: Claude:claude-opus-4-6
>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Please add Tested-by: if you tested it with the tdc patch in 2/2
>
> cheers,
> jamal
Tested-by: Weiming Shi <bestswngs@gmail.com>
Thanks,
Weiming Shi
> > ---
> > net/sched/sch_taprio.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> > index 8e37528119506..a7daf34593e07 100644
> > --- a/net/sched/sch_taprio.c
> > +++ b/net/sched/sch_taprio.c
> > @@ -634,7 +634,7 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> > queue = skb_get_queue_mapping(skb);
> >
> > child = q->qdiscs[queue];
> > - if (unlikely(!child))
> > + if (unlikely(child == &noop_qdisc))
> > return qdisc_drop(skb, sch, to_free);
> >
> > if (taprio_skb_exceeds_queue_max_sdu(sch, skb)) {
> > @@ -717,7 +717,7 @@ static struct sk_buff *taprio_dequeue_from_txq(struct Qdisc *sch, int txq,
> > int len;
> > u8 tc;
> >
> > - if (unlikely(!child))
> > + if (unlikely(child == &noop_qdisc))
> > return NULL;
> >
> > if (TXTIME_ASSIST_IS_ENABLED(q->flags))
> > @@ -2183,6 +2183,9 @@ static int taprio_graft(struct Qdisc *sch, unsigned long cl,
> > if (!dev_queue)
> > return -EINVAL;
> >
> > + if (!new)
> > + new = &noop_qdisc;
> > +
> > if (dev->flags & IFF_UP)
> > dev_deactivate(dev, false);
> >
> > @@ -2196,14 +2199,14 @@ static int taprio_graft(struct Qdisc *sch, unsigned long cl,
> > *old = q->qdiscs[cl - 1];
> > if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
> > WARN_ON_ONCE(dev_graft_qdisc(dev_queue, new) != *old);
> > - if (new)
> > + if (new != &noop_qdisc)
> > qdisc_refcount_inc(new);
> > - if (*old)
> > + if (*old && *old != &noop_qdisc)
> > qdisc_put(*old);
> > }
> >
> > q->qdiscs[cl - 1] = new;
> > - if (new)
> > + if (new != &noop_qdisc)
> > new->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
> >
> > if (dev->flags & IFF_UP)
> > --
> > 2.43.0
> >
^ permalink raw reply
* [PATCH iproute2-next] bash-completion: devlink: Fix dev completion trailing colon
From: Danielle Ratson @ 2026-04-26 11:51 UTC (permalink / raw)
To: netdev; +Cc: jiri, dsahern, Danielle Ratson
Cited commit extended 'devlink dev show' to print the instance index when
the kernel provides DEVLINK_ATTR_INDEX, printing the device handle followed
by a colon and the index. The completion code was using the raw text output
as a word list, causing the device handle to be offered with a trailing
colon, e.g. "pci/0000:01:00.0:" instead of "pci/0000:01:00.0".
Switch to JSON output and extract device names using jq, consistent
with how port completion already works, to reliably get only the device
handle names regardless of what additional attributes are printed.
Fixes: 36252727bfc6 ("devlink: show devlink instance index in dev output")
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
bash-completion/devlink | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/bash-completion/devlink b/bash-completion/devlink
index c053d3d0..7ec6a7cb 100644
--- a/bash-completion/devlink
+++ b/bash-completion/devlink
@@ -31,7 +31,8 @@ _devlink_direct_complete()
case $1 in
dev)
- value=$(devlink dev show 2>/dev/null)
+ value=$(devlink -j dev show 2>/dev/null \
+ | jq -r '.dev | keys[]')
;;
selftests_id)
dev=${words[4]}
--
2.51.0
^ permalink raw reply related
* Re: [PATCH net] net/sched: act_ct: fix skb leak on fragment check failure
From: Jamal Hadi Salim @ 2026-04-26 11:06 UTC (permalink / raw)
To: phx; +Cc: netdev, jiri, horms
In-Reply-To: <CAKvCo-zzTSXJ6vTazSLD=G=SekH_f2=hOHVUogoq=CQeuovxZw@mail.gmail.com>
On Thu, Apr 16, 2026 at 11:56 PM phx <phx0fer@gmail.com> wrote:
>
> Found it through code review. Reproduced it on a 7.0-rc6 kernel
> using a veth pair with act_ct on ingress:
>
> ip netns add ns_ct
> ip link add veth0 type veth peer name veth1
> ip link set veth1 netns ns_ct
> ip link set veth0 up
> ip netns exec ns_ct ip link set veth1 up
> tc qdisc add dev veth0 clsact
> tc filter add dev veth0 ingress protocol ip flower action ct zone 1
>
Please create a test case using tdc as a second patch. And look at
what Sashiko is complaining about and address it if it makes sense...
cheers,
jamal
> Then send truncated IP packets (10 bytes IP, need 20 minimum) from
> the namespace via raw AF_PACKET socket on veth1. This hits
> pskb_may_pull failure in tcf_ct_ipv4_is_fragment -> -EINVAL ->
> out_frag -> TC_ACT_CONSUMED. net/core/dev.c handles TC_ACT_CONSUMED
> by returning NULL without freeing the skb.
>
> Result on unpatched kernel:
> Sent: 222 packets
> skbuff_head_cache: before=6439 after=6663 growth=224
> FAIL: skb leak detected (224 objects leaked)
>
> Attached a test script that automates this. With the fix applied
> (TC_ACT_SHOT for non-EINPROGRESS errors), the skbs get freed and
> the test passes. The test script is generated by AI.
>
> On Thu, Apr 16, 2026 at 11:32 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>> On Thu, Apr 16, 2026 at 9:01 AM Dudu Lu <phx0fer@gmail.com> wrote:
>> >
>> > When tcf_ct_handle_fragments() returns an error other than -EINPROGRESS
>> > (e.g. -EINVAL from malformed fragments), tcf_ct_act() jumps to out_frag
>> > which unconditionally returns TC_ACT_CONSUMED. This tells the caller the
>> > skb was consumed, but it was not freed, leaking one skb per malformed
>> > fragment.
>> >
>> > TC_ACT_CONSUMED is only correct for -EINPROGRESS, where defragmentation
>> > is genuinely in progress and the skb has been queued. For all other
>> > errors the skb is still owned by the caller and must be freed via
>> > TC_ACT_SHOT.
>> >
>> > Fixes: 3f14b377d01d ("net/sched: act_ct: fix skb leak and crash on ooo frags")
>> > Signed-off-by: Dudu Lu <phx0fer@gmail.com>
>>
>> Do you have a reproducer? Always helps adding at least a tdc test.
>> Also: How did you find this issue? was it AI? If yes, please add the
>> tag "Assisted-by:<AI name here>"
>>
>> cheers,
>> jamal
>>
>> > ---
>> > net/sched/act_ct.c | 4 +++-
>> > 1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
>> > index 7d5e50c921a0..870655f682bd 100644
>> > --- a/net/sched/act_ct.c
>> > +++ b/net/sched/act_ct.c
>> > @@ -1107,8 +1107,10 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>> > return retval;
>> >
>> > out_frag:
>> > - if (err != -EINPROGRESS)
>> > + if (err != -EINPROGRESS) {
>> > tcf_action_inc_drop_qstats(&c->common);
>> > + return TC_ACT_SHOT;
>> > + }
>> > return TC_ACT_CONSUMED;
>> >
>> > drop:
>> > --
>> > 2.39.3 (Apple Git-145)
>> >
^ permalink raw reply
* Re: [PATCH net v5 2/2] selftests/tc-testing: add taprio test for class dump after child delete
From: Jamal Hadi Salim @ 2026-04-26 11:00 UTC (permalink / raw)
To: Weiming Shi
Cc: vinicius.gomes, jiri, davem, edumazet, kuba, pabeni, horms,
vladimir.oltean, shuah, xmei5, netdev, linux-kselftest
In-Reply-To: <20260422161958.2517539-4-bestswngs@gmail.com>
On Wed, Apr 22, 2026 at 12:20 PM Weiming Shi <bestswngs@gmail.com> wrote:
>
> Add a regression test for the NULL pointer dereference fixed in the
> previous commit. Before the fix, taprio_graft() stored NULL into
> q->qdiscs[cl - 1] when an explicitly grafted child qdisc was deleted
> via RTM_DELQDISC; the next RTM_GETTCLASS dump then crashed the kernel
> in taprio_dump_class() while reading child->handle.
>
> The test installs a taprio root qdisc on a multi-queue netdevsim
> device, grafts a pfifo child onto class 8001:1, deletes that child,
> and then performs a class dump. On a fixed kernel the dump succeeds
> and all eight taprio classes are listed; on an unpatched kernel the
> class dump crashes, which surfaces as a test failure.
>
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> Assisted-by: Claude:claude-opus-4-6
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
cheers,
jamal
> ---
> .../tc-testing/tc-tests/qdiscs/taprio.json | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> index 557fb074acf0c..cd19d05925e40 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/taprio.json
> @@ -302,5 +302,31 @@
> "$TC qdisc del dev $ETH root",
> "echo \"1\" > /sys/bus/netdevsim/del_device"
> ]
> + },
> + {
> + "id": "c7e1",
> + "name": "Class dump after graft and delete of explicit child qdisc",
> + "category": [
> + "qdisc",
> + "taprio"
> + ],
> + "plugins": {
> + "requires": "nsPlugin"
> + },
> + "setup": [
> + "echo \"1 1 8\" > /sys/bus/netdevsim/new_device",
> + "$TC qdisc replace dev $ETH handle 8001: parent root taprio num_tc 8 map 0 1 2 3 4 5 6 7 queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 base-time 0 sched-entry S ff 20000000 clockid CLOCK_TAI",
> + "$TC qdisc add dev $ETH parent 8001:1 handle 8002: pfifo",
> + "$TC qdisc del dev $ETH parent 8001:1 handle 8002:"
> + ],
> + "cmdUnderTest": "$TC class show dev $ETH",
> + "expExitCode": "0",
> + "verifyCmd": "$TC class show dev $ETH",
> + "matchPattern": "class taprio 8001:[0-9]+ root",
> + "matchCount": "8",
> + "teardown": [
> + "$TC qdisc del dev $ETH root",
> + "echo \"1\" > /sys/bus/netdevsim/del_device"
> + ]
> }
> ]
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH net v5 1/2] net/sched: taprio: fix NULL pointer dereference in class dump
From: Jamal Hadi Salim @ 2026-04-26 10:59 UTC (permalink / raw)
To: Weiming Shi
Cc: vinicius.gomes, jiri, davem, edumazet, kuba, pabeni, horms,
vladimir.oltean, shuah, xmei5, netdev, linux-kselftest
In-Reply-To: <20260422161958.2517539-3-bestswngs@gmail.com>
On Wed, Apr 22, 2026 at 12:20 PM Weiming Shi <bestswngs@gmail.com> wrote:
>
> When a TAPRIO child qdisc is deleted via RTM_DELQDISC, taprio_graft()
> is called with new == NULL and stores NULL into q->qdiscs[cl - 1].
> Subsequent RTM_GETTCLASS dump operations walk all classes via
> taprio_walk() and call taprio_dump_class(), which calls taprio_leaf()
> returning the NULL pointer, then dereferences it to read child->handle,
> causing a kernel NULL pointer dereference.
>
> The bug is reachable with namespace-scoped CAP_NET_ADMIN on any kernel
> with CONFIG_NET_SCH_TAPRIO enabled. On systems with unprivileged user
> namespaces enabled, an unprivileged local user can trigger a kernel
> panic by creating a taprio qdisc inside a new network namespace,
> grafting an explicit child qdisc, deleting it, and requesting a class
> dump. The RTM_GETTCLASS dump itself requires no capability.
>
> Oops: general protection fault, probably for non-canonical address 0xdffffc0000000007: 0000 [#1] SMP KASAN NOPTI
> KASAN: null-ptr-deref in range [0x0000000000000038-0x000000000000003f]
> RIP: 0010:taprio_dump_class (net/sched/sch_taprio.c:2478)
> Call Trace:
> <TASK>
> tc_fill_tclass (net/sched/sch_api.c:1966)
> qdisc_class_dump (net/sched/sch_api.c:2326)
> taprio_walk (net/sched/sch_taprio.c:2514)
> tc_dump_tclass_qdisc (net/sched/sch_api.c:2352)
> tc_dump_tclass_root (net/sched/sch_api.c:2370)
> tc_dump_tclass (net/sched/sch_api.c:2431)
> rtnl_dumpit (net/core/rtnetlink.c:6864)
> netlink_dump (net/netlink/af_netlink.c:2325)
> rtnetlink_rcv_msg (net/core/rtnetlink.c:6959)
> netlink_rcv_skb (net/netlink/af_netlink.c:2550)
> </TASK>
>
> Fix this by substituting &noop_qdisc when new is NULL in
> taprio_graft(), a common pattern used by other qdiscs (e.g.,
> multiq_graft()) to ensure the q->qdiscs[] slots are never NULL.
> This makes control-plane dump paths safe without requiring individual
> NULL checks.
>
> Since the data-plane paths (taprio_enqueue and taprio_dequeue_from_txq)
> previously had explicit NULL guards that would drop/skip the packet
> cleanly, update those checks to test for &noop_qdisc instead. Without
> this, packets would reach taprio_enqueue_one() which increments the root
> qdisc's qlen and backlog before calling the child's enqueue; noop_qdisc
> drops the packet but those counters are never rolled back, permanently
> inflating the root qdisc's statistics.
>
> After this change *old can be a valid qdisc, NULL, or &noop_qdisc.
> Only call qdisc_put(*old) in the first case to avoid decreasing
> noop_qdisc's refcount, which was never increased.
>
> Fixes: 665338b2a7a0 ("net/sched: taprio: dump class stats for the actual q->qdiscs[]")
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> Assisted-by: Claude:claude-opus-4-6
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Please add Tested-by: if you tested it with the tdc patch in 2/2
cheers,
jamal
> ---
> net/sched/sch_taprio.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 8e37528119506..a7daf34593e07 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -634,7 +634,7 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> queue = skb_get_queue_mapping(skb);
>
> child = q->qdiscs[queue];
> - if (unlikely(!child))
> + if (unlikely(child == &noop_qdisc))
> return qdisc_drop(skb, sch, to_free);
>
> if (taprio_skb_exceeds_queue_max_sdu(sch, skb)) {
> @@ -717,7 +717,7 @@ static struct sk_buff *taprio_dequeue_from_txq(struct Qdisc *sch, int txq,
> int len;
> u8 tc;
>
> - if (unlikely(!child))
> + if (unlikely(child == &noop_qdisc))
> return NULL;
>
> if (TXTIME_ASSIST_IS_ENABLED(q->flags))
> @@ -2183,6 +2183,9 @@ static int taprio_graft(struct Qdisc *sch, unsigned long cl,
> if (!dev_queue)
> return -EINVAL;
>
> + if (!new)
> + new = &noop_qdisc;
> +
> if (dev->flags & IFF_UP)
> dev_deactivate(dev, false);
>
> @@ -2196,14 +2199,14 @@ static int taprio_graft(struct Qdisc *sch, unsigned long cl,
> *old = q->qdiscs[cl - 1];
> if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
> WARN_ON_ONCE(dev_graft_qdisc(dev_queue, new) != *old);
> - if (new)
> + if (new != &noop_qdisc)
> qdisc_refcount_inc(new);
> - if (*old)
> + if (*old && *old != &noop_qdisc)
> qdisc_put(*old);
> }
>
> q->qdiscs[cl - 1] = new;
> - if (new)
> + if (new != &noop_qdisc)
> new->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
>
> if (dev->flags & IFF_UP)
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH net v2] ipv6: Implement limits on extension header parsing
From: saeed bishara @ 2026-04-26 10:56 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Justin Iurman, kuba, edumazet, dsahern, tom,
willemdebruijn.kernel, idosch, pabeni, netdev
In-Reply-To: <90c7de29-2641-413d-9d5f-5eb323cf875c@iogearbox.net>
On Sun, Apr 26, 2026 at 1:38 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Hi Justin,
>
> On 4/25/26 12:19 PM, Justin Iurman wrote:
> > On 4/25/26 09:55, Daniel Borkmann wrote:
> >> ipv6_{skip_exthdr,find_hdr}() and ip6_{tnl_parse_tlv_enc_lim,
> >> protocol_deliver_rcu}() iterate over IPv6 extension headers until they
> >> find a non-extension-header protocol or run out of packet data. The
> >> loops have no iteration counter, relying solely on the packet length
> >> to bound them. For a crafted packet with 8-byte extension headers
> >> filling a 64KB jumbogram, this means a worst case of up to ~8k
> >> iterations with a skb_header_pointer call each. ipv6_skip_exthdr(),
> >> for example, is used where it parses the inner quoted packet inside
> >> an incoming ICMPv6 error:
> >>
> >> - icmpv6_rcv
> >> - checksum validation
> >> - case ICMPV6_DEST_UNREACH
> >> - icmpv6_notify
> >> - pskb_may_pull() <- pull inner IPv6 header
> >> - ipv6_skip_exthdr() <- iterates here
> >> - pskb_may_pull()
> >> - ipprot->err_handler() <- sk lookup
> >>
> >> The per-iteration cost of ipv6_skip_exthdr itself is generally
> >> light, but skb_header_pointer becomes more costly on reassembled
> >> packets: the first ~1232 bytes of the inner packet are in the skb's
> >> linear area, but the remaining ~63KB are in the frag_list where
> >> skb_copy_bits is needed to read data.
> >>
> >> Add a configurable limit via a new sysctl net.ipv6.max_ext_hdrs_number
> >> (default 8, minimum 1). All four extension header walking functions
> >> are bound by this limit. The sysctl is in line with commit 47d3d7ac656a
> >> ("ipv6: Implement limits on Hop-by-Hop and Destination options").
> >> As documented, init_net is used to derive max_ext_hdrs_number to
> >> be consistent given a net cannot always reliably be retrieved.
> >>
> >> Note that the check in ip6_protocol_deliver_rcu() happens right
> >> before the goto resubmit, such that we don't have to have a test
> >> for ipv6_ext_hdr() in the fast-path.
> >>
> >> There's an ongoing IETF draft-iurman-6man-eh-occurrences to enforce
> >> IPv6 extension headers ordering and occurrence. The latter also
> >> discusses security implications. As per RFC8200 section 4.1, the
> >> occurrence rules for extension headers provide a practical upper
> >> bound, thus 8 was used as the default.
> >>
> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> >> ---
> >> v1->v2:
> >> - Set the default to 8 (Justin)
> >> - Update IETF references (Justin)
> >> - Add core path coverage as well (Justin)
> >>
> >> Documentation/networking/ip-sysctl.rst | 7 +++++++
> >> include/net/dropreason-core.h | 6 ++++++
> >> include/net/ipv6.h | 2 ++
> >> include/net/netns/ipv6.h | 1 +
> >> net/ipv6/af_inet6.c | 1 +
> >> net/ipv6/exthdrs_core.c | 11 +++++++++++
> >> net/ipv6/ip6_input.c | 6 ++++++
> >> net/ipv6/ip6_tunnel.c | 5 +++++
> >> net/ipv6/sysctl_net_ipv6.c | 8 ++++++++
> >> 9 files changed, 47 insertions(+)
> >>
> >
> > [snip]
> >
> >> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> >> index 967b07aeb683..a5bbbc16e8d7 100644
> >> --- a/net/ipv6/ip6_input.c
> >> +++ b/net/ipv6/ip6_input.c
> >> @@ -403,8 +403,10 @@ INDIRECT_CALLABLE_DECLARE(int tcp_v6_rcv(struct sk_buff *));
> >> void ip6_protocol_deliver_rcu(struct net *net, struct sk_buff *skb, int nexthdr,
> >> bool have_final)
> >> {
> >> + int exthdr_max = READ_ONCE(init_net.ipv6.sysctl.max_ext_hdrs_cnt);
> >> const struct inet6_protocol *ipprot;
> >> struct inet6_dev *idev;
> >> + int exthdr_cnt = 0;
> >> unsigned int nhoff;
> >> SKB_DR(reason);
> >> bool raw;
> >> @@ -487,6 +489,10 @@ void ip6_protocol_deliver_rcu(struct net *net, struct sk_buff *skb, int nexthdr,
> >> nexthdr = ret;
> >> goto resubmit_final;
> >> } else {
> >> + if (unlikely(exthdr_cnt++ >= exthdr_max)) {
from performance perspective, isn't it better to have single variable
that initialized to max_ext_hdrs_cnt then decremented until reaches
zero? that will take less cpu cycles and variables
> >> + SKB_DR_SET(reason, IPV6_TOO_MANY_EXTHDRS);
> >> + goto discard;
> >> + }
> >> goto resubmit;
> >> }
> >> } else if (ret == 0) {
> >
> > The hop-by-hop options header (if present) is not taken into account based on the above. However, the max number of extension headers (implicitly 7***, as per RFC 8200 Section 4.1) must include it. I suggest adding this at the beginning of ip6_protocol_deliver_rcu():
> >
> > struct inet6_skb_parm *opt = IP6CB(skb);
> >
> > if (opt->flags & IP6SKB_HOPBYHOP)
> > exthdr_cnt++;
> >
> > *** FYI, rounding to 8 is fine for this fix
>
> Ok, ack, I'll look into adding that in a v3.
>
> >> diff --git a/net/ipv6/sysctl_net_ipv6.c b/net/ipv6/sysctl_net_ipv6.c
> >> index d2cd33e2698d..93f865545a7c 100644
> >> --- a/net/ipv6/sysctl_net_ipv6.c
> >> +++ b/net/ipv6/sysctl_net_ipv6.c
> >> @@ -135,6 +135,14 @@ static struct ctl_table ipv6_table_template[] = {
> >> .extra1 = SYSCTL_ZERO,
> >> .extra2 = &flowlabel_reflect_max,
> >> },
> >> + {
> >> + .procname = "max_ext_hdrs_number",
> >> + .data = &init_net.ipv6.sysctl.max_ext_hdrs_cnt,
> >> + .maxlen = sizeof(int),
> >> + .mode = 0644,
> >> + .proc_handler = proc_dointvec_minmax,
> >> + .extra1 = SYSCTL_ONE,
> >> + },
> >> {
> >> .procname = "max_dst_opts_number",
> >> .data = &init_net.ipv6.sysctl.max_dst_opts_cnt,
> >
> > I've given it a lot of thought. I came to the conclusion that we should use a hard-coded value here as well (just like we did for 076b8cad77aa, with the same logic), not a sysctl. IMO, the main reason is that it provides as is a suitable security fix to be backported, i.e., the max value is the max number of EHs allowed by RFC 8200, Section 4.1. Also, we remain consistent with draft-iurman-6man-eh-occurrences (I think Tom is about to send a revision of the series soon for net-next). What this series does is not only enforcing ordering, but also verifying the specific number of occurrences for each type of Extension Header. Which is totally compatible with what this patch does, i.e., limiting the total number of Extension Headers (regardless of their types) to 8. I guess what I'm trying to say is that it seems like a good plan/compromise and that the aforementioned series would build perfectly on top of this fix.
>
>
> Initially, I had a hard-coded constant (when it was still 32), but Eric's comment
> was to rather go with a sysctl, such that if someone unexpectedly complains, then
> there is still a chance for that person to fix it up via sysctl without having to
> rebuild the kernel. I'm okay either way, but presumably given we're now being more
> "aggressive" into lowering the default to 8 rather than 32 then having such a fall-
> back is probably better.
>
> Thanks,
> Daniel
>
^ permalink raw reply
* Re: [PATCH net V3 1/4] net/mlx5: SD: Serialize init/cleanup
From: Shay Drori @ 2026-04-26 10:46 UTC (permalink / raw)
To: Tariq Toukan, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Lunn, David S. Miller
Cc: Saeed Mahameed, Mark Bloch, Leon Romanovsky, Simon Horman,
Kees Cook, Patrisious Haddad, Parav Pandit, Gal Pressman, netdev,
linux-rdma, linux-kernel, Dragos Tatulea
In-Reply-To: <20260423123104.201552-2-tariqt@nvidia.com>
On 23/04/2026 15:31, Tariq Toukan wrote:
> From: Shay Drory <shayd@nvidia.com>
>
> mlx5_sd_init() / mlx5_sd_cleanup() may run from multiple PFs in the same
> Socket-Direct group. This can cause the SD bring-up/tear-down sequence
> to be executed more than once or interleaved across PFs.
>
> Protect SD init/cleanup with mlx5_devcom_comp_lock() and track the SD
> group state on the primary device. Skip init if the primary is already
> UP, and skip cleanup unless the primary is UP.
Sashiko:
"The commit message mentions skipping cleanup unless the primary is UP.
However, it appears this state check is missing from mlx5_sd_cleanup()
in the diff below."
The above sentence is leftover and should be removed.
will drop in next version.
>
> In addition, move mlx5_devcom_comp_set_ready(false) from sd_unregister()
> into the cleanup's locked section. A concurrent init acquiring the
> devcom lock will now observe devcom is no longer ready and bail out
> immediately.
>
> Fixes: 381978d28317 ("net/mlx5e: Create single netdev per SD group")
> Signed-off-by: Shay Drory <shayd@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/lib/sd.c | 32 +++++++++++++++----
> 1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> index 762c783156b4..96b4316f570e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> @@ -18,6 +18,7 @@ struct mlx5_sd {
> u8 host_buses;
> struct mlx5_devcom_comp_dev *devcom;
> struct dentry *dfs;
> + u8 state;
> bool primary;
> union {
> struct { /* primary */
> @@ -31,6 +32,11 @@ struct mlx5_sd {
> };
> };
>
> +enum mlx5_sd_state {
> + MLX5_SD_STATE_DOWN = 0,
> + MLX5_SD_STATE_UP,
> +};
> +
> static int mlx5_sd_get_host_buses(struct mlx5_core_dev *dev)
> {
> struct mlx5_sd *sd = mlx5_get_sd(dev);
> @@ -270,9 +276,6 @@ static void sd_unregister(struct mlx5_core_dev *dev)
> {
> struct mlx5_sd *sd = mlx5_get_sd(dev);
>
> - mlx5_devcom_comp_lock(sd->devcom);
> - mlx5_devcom_comp_set_ready(sd->devcom, false);
> - mlx5_devcom_comp_unlock(sd->devcom);
> mlx5_devcom_unregister_component(sd->devcom);
> }
>
> @@ -426,6 +429,7 @@ int mlx5_sd_init(struct mlx5_core_dev *dev)
> struct mlx5_core_dev *primary, *pos, *to;
> struct mlx5_sd *sd = mlx5_get_sd(dev);
> u8 alias_key[ACCESS_KEY_LEN];
> + struct mlx5_sd *primary_sd;
> int err, i;
>
> err = sd_init(dev);
> @@ -440,10 +444,15 @@ int mlx5_sd_init(struct mlx5_core_dev *dev)
> if (err)
> goto err_sd_cleanup;
>
> + mlx5_devcom_comp_lock(sd->devcom);
> if (!mlx5_devcom_comp_is_ready(sd->devcom))
> - return 0;
> + goto out;
Sashiko:
"Can primary be NULL here?
In sd_register(), the devcom ready state is published under the devcom
lock, but the lock is then released before the peer_sd->primary_dev
pointers are initialized.
If a concurrent thread executing mlx5_sd_init() or mlx5_sd_cleanup()
acquires the lock and observes the ready state, could it read an
uninitialized primary_dev before the loop in sd_register() completes?"
No, this is impossible. concurrent init will always set primary before
accessing it, and cleanup is always after successful init, so again-
primary is set.
and the next comment is also impossible seqence
>
> primary = mlx5_sd_get_primary(dev);
> + primary_sd = mlx5_get_sd(primary);
> +
> + if (primary_sd->state != MLX5_SD_STATE_DOWN)
> + goto out;
>
> for (i = 0; i < ACCESS_KEY_LEN; i++)
> alias_key[i] = get_random_u8();
> @@ -472,6 +481,9 @@ int mlx5_sd_init(struct mlx5_core_dev *dev)
> sd->group_id, mlx5_devcom_comp_get_size(sd->devcom));
> sd_print_group(primary);
>
> + primary_sd->state = MLX5_SD_STATE_UP;
> +out:
> + mlx5_devcom_comp_unlock(sd->devcom);
> return 0;
>
> err_unset_secondaries:
> @@ -481,6 +493,8 @@ int mlx5_sd_init(struct mlx5_core_dev *dev)
> sd_cmd_unset_primary(primary);
> debugfs_remove_recursive(sd->dfs);
> err_sd_unregister:
> + mlx5_devcom_comp_set_ready(sd->devcom, false);
> + mlx5_devcom_comp_unlock(sd->devcom);
> sd_unregister(dev);
> err_sd_cleanup:
> sd_cleanup(dev);
> @@ -491,22 +505,28 @@ void mlx5_sd_cleanup(struct mlx5_core_dev *dev)
> {
> struct mlx5_sd *sd = mlx5_get_sd(dev);
> struct mlx5_core_dev *primary, *pos;
> + struct mlx5_sd *primary_sd;
> int i;
>
> if (!sd)
> return;
>
> + mlx5_devcom_comp_lock(sd->devcom);
> if (!mlx5_devcom_comp_is_ready(sd->devcom))
> - goto out;
> + goto out_unlock;
>
> primary = mlx5_sd_get_primary(dev);
> + primary_sd = mlx5_get_sd(primary);
> mlx5_sd_for_each_secondary(i, primary, pos)
> sd_cmd_unset_secondary(pos);
> sd_cmd_unset_primary(primary);
> debugfs_remove_recursive(sd->dfs);
>
> sd_info(primary, "group id %#x, uncombined\n", sd->group_id);
> -out:
> + primary_sd->state = MLX5_SD_STATE_DOWN;
> + mlx5_devcom_comp_set_ready(sd->devcom, false);
> +out_unlock:
> + mlx5_devcom_comp_unlock(sd->devcom);
> sd_unregister(dev);
> sd_cleanup(dev);
> }
^ permalink raw reply
* Re: [PATCH net V3 3/4] net/mlx5e: SD, Fix missing cleanup on probe/resume error
From: Shay Drori @ 2026-04-26 10:45 UTC (permalink / raw)
To: Tariq Toukan, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Lunn, David S. Miller
Cc: Saeed Mahameed, Mark Bloch, Leon Romanovsky, Simon Horman,
Kees Cook, Patrisious Haddad, Parav Pandit, Gal Pressman, netdev,
linux-rdma, linux-kernel, Dragos Tatulea
In-Reply-To: <20260423123104.201552-4-tariqt@nvidia.com>
On 23/04/2026 15:31, Tariq Toukan wrote:
> From: Shay Drory <shayd@nvidia.com>
>
> When _mlx5e_probe() or _mlx5e_resume() fails, the preceding
> successful mlx5_sd_init() is not undone, leaving the SD group in an
> UP state without a matching cleanup.
>
> Call mlx5_sd_cleanup() on the error path so the SD state is reset.
>
> Fixes: 381978d28317 ("net/mlx5e: Create single netdev per SD group")
> Signed-off-by: Shay Drory <shayd@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_main.c | 22 +++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 5a46870c4b74..9c340ad2fe09 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -6774,9 +6774,16 @@ static int mlx5e_resume(struct auxiliary_device *adev)
> return err;
>
> actual_adev = mlx5_sd_get_adev(mdev, adev, edev->idx);
> - if (actual_adev)
> - return _mlx5e_resume(actual_adev);
> + if (actual_adev) {
> + err = _mlx5e_resume(actual_adev);
> + if (err)
> + goto sd_cleanup;
> + }
> return 0;
> +
> +sd_cleanup:
> + mlx5_sd_cleanup(mdev);
> + return err;
> }
Sashiko:
"If _mlx5e_resume() fails when called from a secondary device's
mlx5e_resume(),
mlx5_sd_cleanup() is called on the secondary device. This frees the sd
struct
and marks the devcom group as not ready.
Since this is a resume failure, the secondary device remains bound to the
driver. Later, when the driver is unbound, mlx5e_remove() is invoked on the
secondary device.
Could this result in a NULL pointer dereference?
When mlx5e_remove() calls mlx5_sd_get_adev(), it returns the secondary adev
itself because the sd struct was already freed:
mlx5_sd_get_adev() {
...
if (!sd)
return adev;
...
}
This leads to _mlx5e_remove() being erroneously called on the secondary
device. Inside _mlx5e_remove(), auxiliary_get_drvdata() is called.
Because drvdata is only set on the primary device during probe, this
returns NULL. Dereferencing mlx5e_dev->netdev would then cause a kernel
panic."
That is correct. In next version mlx5_sd_cleanup() won't be called in
mlx5e_resume(), and the commit message will elaborate to explain the
different handling between probe and resume.
>
> static int _mlx5e_suspend(struct auxiliary_device *adev, bool pre_netdev_reg)
> @@ -6912,9 +6919,16 @@ static int mlx5e_probe(struct auxiliary_device *adev,
> return err;
>
> actual_adev = mlx5_sd_get_adev(mdev, adev, edev->idx);
> - if (actual_adev)
> - return _mlx5e_probe(actual_adev);
> + if (actual_adev) {
> + err = _mlx5e_probe(actual_adev);
> + if (err)
> + goto sd_cleanup;
> + }
> return 0;
> +
> +sd_cleanup:
> + mlx5_sd_cleanup(mdev);
> + return err;
> }
>
> static void _mlx5e_remove(struct auxiliary_device *adev)
^ permalink raw reply
* Re: [PATCH net v2] ipv6: Implement limits on extension header parsing
From: Daniel Borkmann @ 2026-04-26 10:38 UTC (permalink / raw)
To: Justin Iurman, kuba
Cc: edumazet, dsahern, tom, willemdebruijn.kernel, idosch, pabeni,
netdev
In-Reply-To: <b50025ba-c59d-4575-a790-fdaf0a48961d@gmail.com>
Hi Justin,
On 4/25/26 12:19 PM, Justin Iurman wrote:
> On 4/25/26 09:55, Daniel Borkmann wrote:
>> ipv6_{skip_exthdr,find_hdr}() and ip6_{tnl_parse_tlv_enc_lim,
>> protocol_deliver_rcu}() iterate over IPv6 extension headers until they
>> find a non-extension-header protocol or run out of packet data. The
>> loops have no iteration counter, relying solely on the packet length
>> to bound them. For a crafted packet with 8-byte extension headers
>> filling a 64KB jumbogram, this means a worst case of up to ~8k
>> iterations with a skb_header_pointer call each. ipv6_skip_exthdr(),
>> for example, is used where it parses the inner quoted packet inside
>> an incoming ICMPv6 error:
>>
>> - icmpv6_rcv
>> - checksum validation
>> - case ICMPV6_DEST_UNREACH
>> - icmpv6_notify
>> - pskb_may_pull() <- pull inner IPv6 header
>> - ipv6_skip_exthdr() <- iterates here
>> - pskb_may_pull()
>> - ipprot->err_handler() <- sk lookup
>>
>> The per-iteration cost of ipv6_skip_exthdr itself is generally
>> light, but skb_header_pointer becomes more costly on reassembled
>> packets: the first ~1232 bytes of the inner packet are in the skb's
>> linear area, but the remaining ~63KB are in the frag_list where
>> skb_copy_bits is needed to read data.
>>
>> Add a configurable limit via a new sysctl net.ipv6.max_ext_hdrs_number
>> (default 8, minimum 1). All four extension header walking functions
>> are bound by this limit. The sysctl is in line with commit 47d3d7ac656a
>> ("ipv6: Implement limits on Hop-by-Hop and Destination options").
>> As documented, init_net is used to derive max_ext_hdrs_number to
>> be consistent given a net cannot always reliably be retrieved.
>>
>> Note that the check in ip6_protocol_deliver_rcu() happens right
>> before the goto resubmit, such that we don't have to have a test
>> for ipv6_ext_hdr() in the fast-path.
>>
>> There's an ongoing IETF draft-iurman-6man-eh-occurrences to enforce
>> IPv6 extension headers ordering and occurrence. The latter also
>> discusses security implications. As per RFC8200 section 4.1, the
>> occurrence rules for extension headers provide a practical upper
>> bound, thus 8 was used as the default.
>>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>> v1->v2:
>> - Set the default to 8 (Justin)
>> - Update IETF references (Justin)
>> - Add core path coverage as well (Justin)
>>
>> Documentation/networking/ip-sysctl.rst | 7 +++++++
>> include/net/dropreason-core.h | 6 ++++++
>> include/net/ipv6.h | 2 ++
>> include/net/netns/ipv6.h | 1 +
>> net/ipv6/af_inet6.c | 1 +
>> net/ipv6/exthdrs_core.c | 11 +++++++++++
>> net/ipv6/ip6_input.c | 6 ++++++
>> net/ipv6/ip6_tunnel.c | 5 +++++
>> net/ipv6/sysctl_net_ipv6.c | 8 ++++++++
>> 9 files changed, 47 insertions(+)
>>
>
> [snip]
>
>> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
>> index 967b07aeb683..a5bbbc16e8d7 100644
>> --- a/net/ipv6/ip6_input.c
>> +++ b/net/ipv6/ip6_input.c
>> @@ -403,8 +403,10 @@ INDIRECT_CALLABLE_DECLARE(int tcp_v6_rcv(struct sk_buff *));
>> void ip6_protocol_deliver_rcu(struct net *net, struct sk_buff *skb, int nexthdr,
>> bool have_final)
>> {
>> + int exthdr_max = READ_ONCE(init_net.ipv6.sysctl.max_ext_hdrs_cnt);
>> const struct inet6_protocol *ipprot;
>> struct inet6_dev *idev;
>> + int exthdr_cnt = 0;
>> unsigned int nhoff;
>> SKB_DR(reason);
>> bool raw;
>> @@ -487,6 +489,10 @@ void ip6_protocol_deliver_rcu(struct net *net, struct sk_buff *skb, int nexthdr,
>> nexthdr = ret;
>> goto resubmit_final;
>> } else {
>> + if (unlikely(exthdr_cnt++ >= exthdr_max)) {
>> + SKB_DR_SET(reason, IPV6_TOO_MANY_EXTHDRS);
>> + goto discard;
>> + }
>> goto resubmit;
>> }
>> } else if (ret == 0) {
>
> The hop-by-hop options header (if present) is not taken into account based on the above. However, the max number of extension headers (implicitly 7***, as per RFC 8200 Section 4.1) must include it. I suggest adding this at the beginning of ip6_protocol_deliver_rcu():
>
> struct inet6_skb_parm *opt = IP6CB(skb);
>
> if (opt->flags & IP6SKB_HOPBYHOP)
> exthdr_cnt++;
>
> *** FYI, rounding to 8 is fine for this fix
Ok, ack, I'll look into adding that in a v3.
>> diff --git a/net/ipv6/sysctl_net_ipv6.c b/net/ipv6/sysctl_net_ipv6.c
>> index d2cd33e2698d..93f865545a7c 100644
>> --- a/net/ipv6/sysctl_net_ipv6.c
>> +++ b/net/ipv6/sysctl_net_ipv6.c
>> @@ -135,6 +135,14 @@ static struct ctl_table ipv6_table_template[] = {
>> .extra1 = SYSCTL_ZERO,
>> .extra2 = &flowlabel_reflect_max,
>> },
>> + {
>> + .procname = "max_ext_hdrs_number",
>> + .data = &init_net.ipv6.sysctl.max_ext_hdrs_cnt,
>> + .maxlen = sizeof(int),
>> + .mode = 0644,
>> + .proc_handler = proc_dointvec_minmax,
>> + .extra1 = SYSCTL_ONE,
>> + },
>> {
>> .procname = "max_dst_opts_number",
>> .data = &init_net.ipv6.sysctl.max_dst_opts_cnt,
>
> I've given it a lot of thought. I came to the conclusion that we should use a hard-coded value here as well (just like we did for 076b8cad77aa, with the same logic), not a sysctl. IMO, the main reason is that it provides as is a suitable security fix to be backported, i.e., the max value is the max number of EHs allowed by RFC 8200, Section 4.1. Also, we remain consistent with draft-iurman-6man-eh-occurrences (I think Tom is about to send a revision of the series soon for net-next). What this series does is not only enforcing ordering, but also verifying the specific number of occurrences for each type of Extension Header. Which is totally compatible with what this patch does, i.e., limiting the total number of Extension Headers (regardless of their types) to 8. I guess what I'm trying to say is that it seems like a good plan/compromise and that the aforementioned series would build perfectly on top of this fix.
Initially, I had a hard-coded constant (when it was still 32), but Eric's comment
was to rather go with a sysctl, such that if someone unexpectedly complains, then
there is still a chance for that person to fix it up via sysctl without having to
rebuild the kernel. I'm okay either way, but presumably given we're now being more
"aggressive" into lowering the default to 8 rather than 32 then having such a fall-
back is probably better.
Thanks,
Daniel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox