* Re: [PATCH net-next V9 04/14] devlink: Decouple rate storage from associated devlink object
From: Jakub Kicinski @ 2026-03-31 2:08 UTC (permalink / raw)
To: tariqt
Cc: Jakub Kicinski, edumazet, pabeni, andrew+netdev, davem,
donald.hunter, horms, jiri, corbet, skhan, saeedm, leon, mbloch,
shuah, chuck.lever, matttbe, cjubran, cratiu, dtatulea,
jacob.e.keller, shshitrit, daniel.zahka, parav, ajayachandra,
kees, shayd, danielj, moshe, willemb, dw, petrm, sdf, daniel, joe,
razor, vadim.fedorenko, mst, antonio, allison.henderson,
minhquangbui99, noren, netdev, linux-kernel, linux-doc,
linux-rdma, linux-kselftest, gal, jiri
In-Reply-To: <20260326065949.44058-5-tariqt@nvidia.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
devlink: Decouple rate storage from associated devlink object
Devlink rate leafs and nodes were stored in their respective devlink
objects pointed to by devlink_rate->devlink.
> diff --git a/net/devlink/rate.c b/net/devlink/rate.c
> index 4781429109195..9ebbc72130c6a 100644
> --- a/net/devlink/rate.c
> +++ b/net/devlink/rate.c
[ ... ]
> @@ -648,8 +702,9 @@ int devlink_nl_rate_new_doit(struct sk_buff *skb, struct genl_info *info)
> goto err_rate_set;
>
> refcount_set(&rate_node->refcnt, 1);
> - list_add(&rate_node->list, &devlink->rate_list);
> + list_add(&rate_node->list, &rate_devlink->rate_list);
> devlink_rate_notify(rate_node, DEVLINK_CMD_RATE_NEW);
> + devl_rate_unlock(devlink);
> return 0;
>
> err_rate_set:
> ops->rate_node_del(rate_node, rate_node->priv, info->extack);
> @@ -658,6 +713,8 @@ int devlink_nl_rate_new_doit(struct sk_buff *skb, struct genl_info *info)
> kfree(rate_node->name);
> err_strdup:
> kfree(rate_node);
> +unlock:
> + devl_rate_unlock(devlink);
> return err;
> }
If devlink_nl_rate_set() fails after it successfully sets the parent node
and increments the parent's refcount, does this error path permanently
leak that reference since it frees the node without decrementing it?
[ ... ]
> @@ -716,14 +785,20 @@ devl_rate_node_create(struct devlink *devlink, void *priv, char *node_name,
> struct devlink_rate *parent)
> {
> struct devlink_rate *rate_node;
> + struct devlink *rate_devlink;
>
> + rate_devlink = devl_rate_lock(devlink);
> rate_node = devlink_rate_node_get_by_name(devlink, node_name);
> - if (!IS_ERR(rate_node))
> - return ERR_PTR(-EEXIST);
> + if (!IS_ERR(rate_node)) {
> + rate_node = ERR_PTR(-EEXIST);
> + goto unlock;
> + }
>
> rate_node = kzalloc_obj(*rate_node);
> - if (!rate_node)
> - return ERR_PTR(-ENOMEM);
> + if (!rate_node) {
> + rate_node = ERR_PTR(-ENOMEM);
> + goto unlock;
> + }
>
> if (parent) {
> rate_node->parent = parent;
> @@ -737,12 +812,15 @@ devl_rate_node_create(struct devlink *devlink, void *priv, char *node_name,
> rate_node->name = kstrdup(node_name, GFP_KERNEL);
> if (!rate_node->name) {
> kfree(rate_node);
> - return ERR_PTR(-ENOMEM);
> + rate_node = ERR_PTR(-ENOMEM);
> + goto unlock;
> }
Similarly, if kstrdup() fails here, does this leak the parent's refcount
since it was already incremented earlier in the function?
^ permalink raw reply
* Re: [PATCH net-next V9 02/14] devlink: Add helpers to lock nested-in instances
From: Jakub Kicinski @ 2026-03-31 2:08 UTC (permalink / raw)
To: tariqt
Cc: Jakub Kicinski, edumazet, pabeni, andrew+netdev, davem,
donald.hunter, horms, jiri, corbet, skhan, saeedm, leon, mbloch,
shuah, chuck.lever, matttbe, cjubran, cratiu, dtatulea,
jacob.e.keller, shshitrit, daniel.zahka, parav, ajayachandra,
kees, shayd, danielj, moshe, willemb, dw, petrm, sdf, daniel, joe,
razor, vadim.fedorenko, mst, antonio, allison.henderson,
minhquangbui99, noren, netdev, linux-kernel, linux-doc,
linux-rdma, linux-kselftest, gal, jiri
In-Reply-To: <20260326065949.44058-3-tariqt@nvidia.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
devlink: Add helpers to lock nested-in instances
Upcoming code will need to obtain a reference to locked nested-in
devlink instances. Add helpers to lock, obtain an already locked
reference and unlock/unref the nested-in instance.
> diff --git a/net/devlink/core.c b/net/devlink/core.c
> index eeb6a71f5f56e..db11248df7120 100644
> --- a/net/devlink/core.c
> +++ b/net/devlink/core.c
[ ... ]
> +/* Returns the nested in devlink object and validates its lock is held. */
> +struct devlink *devlink_nested_in_get_locked(struct devlink_rel *rel)
> +{
> + struct devlink *devlink;
> + unsigned long index;
> +
> + if (!rel)
> + return NULL;
> + index = rel->nested_in.devlink_index;
> + devlink = xa_find(&devlinks, &index, index, DEVLINK_REGISTERED);
> + if (devlink)
> + devl_assert_locked(devlink);
Can this dereference a freed pointer if the lock is not actually held?
xa_find() acquires and releases the RCU read lock internally. When it returns,
the devlink pointer is no longer protected by RCU or a reference count.
If a caller invokes this helper without holding the devlink lock (which is the
exact scenario the assertion is designed to detect), another thread could
concurrently unregister and free the devlink.
If the RCU grace period expires before the assertion executes,
devl_assert_locked() would dereference freed memory. Should the lookup and
assertion be enclosed within an explicit rcu_read_lock() block to safely
assert the lock?
> + return devlink;
> +}
> +
> +void devlink_nested_in_put_unlock(struct devlink_rel *rel)
> +{
> + struct devlink *devlink = devlink_nested_in_get_locked(rel);
Could this corrupt the mutex state and cause a refcount underflow?
This helper takes a devlink_rel pointer instead of the devlink pointer
acquired by devlink_nested_in_get_lock(), and performs a secondary global
lookup to find the devlink.
If a caller mistakenly calls this in an error cleanup path where they did not
actually acquire the lock, the global xa_find() will still locate the
registered devlink. This would execute devl_unlock() and devlink_put() on a
devlink the current thread does not own.
Would it be safer for unlock/put helpers to take the exact pointer returned by
the lock/get helper to ensure safe resource cleanup?
> +
> + if (devlink) {
> + devl_unlock(devlink);
> + devlink_put(devlink);
> + }
> +}
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH net-next v7 4/5] tls: Suppress spurious saved_data_ready on all receive paths
From: Chuck Lever @ 2026-03-31 2:06 UTC (permalink / raw)
To: Sabrina Dubroca
Cc: john.fastabend, Jakub Kicinski, netdev, kernel-tls-handshake,
Chuck Lever, Alistair Francis, Hannes Reinecke
In-Reply-To: <acqMIKWFmx6B1lD8@krikkit>
On Mon, Mar 30, 2026, at 10:43 AM, Sabrina Dubroca wrote:
> 2026-03-28, 11:17:11 -0400, Chuck Lever wrote:
>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
>> index 5fdd43a55f1e..8fb2f2a93846 100644
>> --- a/net/tls/tls_sw.c
>> +++ b/net/tls/tls_sw.c
>> @@ -1373,7 +1373,11 @@ tls_rx_rec_wait(struct sock *sk, struct sk_psock *psock, bool nonblock,
>> return ret;
>>
>> if (!skb_queue_empty(&sk->sk_receive_queue)) {
>> - tls_strp_check_rcv(&ctx->strp);
>> + /* Defer notification to the exit point;
>> + * this thread will consume the record
>> + * directly.
>> + */
>
> That's a really nice improvement over the comment you had here
> before. Thanks.
>
>> + tls_strp_check_rcv(&ctx->strp, false);
>> if (tls_strp_msg_ready(ctx))
>> break;
>> }
>
> [...]
>> @@ -2142,7 +2146,7 @@ int tls_sw_recvmsg(struct sock *sk,
>> err = tls_record_content_type(msg, tls_msg(darg.skb), &control);
>> if (err <= 0) {
>> DEBUG_NET_WARN_ON_ONCE(darg.zc);
>> - tls_rx_rec_done(ctx);
>> + tls_rx_rec_release(ctx);
>> put_on_rx_list_err:
>> __skb_queue_tail(&ctx->rx_list, darg.skb);
>> goto recv_end;
>> @@ -2156,7 +2160,8 @@ int tls_sw_recvmsg(struct sock *sk,
>> /* TLS 1.3 may have updated the length by more than overhead */
>> rxm = strp_msg(darg.skb);
>> chunk = rxm->full_len;
>> - tls_rx_rec_done(ctx);
>> + tls_rx_rec_release(ctx);
>> + tls_strp_check_rcv(&ctx->strp, false);
>
> Not strictly an objection against your patch, but after those changes,
> calling tls_strp_check_rcv() at this point in tls_sw_recvmsg() is
> starting to look like a leftover from the transition from generic strp
> to custom strp. We're not going to do anything with the next record
> until we call tls_rx_rec_wait(), so it seems it would fit better
> before the loop in tls_rx_rec_wait().
>
>
> [...]
>> @@ -2290,7 +2296,7 @@ ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos,
>> if (err < 0)
>> goto splice_read_end;
>>
>> - tls_rx_rec_done(ctx);
>> + tls_rx_rec_release(ctx);
>> skb = darg.skb;
>> }
>>
>> @@ -2317,6 +2323,7 @@ ssize_t tls_sw_splice_read(struct socket *sock, loff_t *ppos,
>> consume_skb(skb);
>>
>> splice_read_end:
>> + tls_strp_check_rcv(&ctx->strp, true);
>
> This is in effect adding a
>
> if (strp->msg_ready)
> tls_rx_msg_ready(strp);
>
> [a bit more than that]
> in case we dequeue from the rx_list but don't use the record (or only
> part of it).
>
> I wonder if that should be seen as a problem (another spurious wakeup)
> or an improvement (wake up because there's data ready)? Or if we
> should wake up anyway if we exit with rx_list non-empty, regardless or
> the parser's state, since there is data to read (tls_sk_poll looks at
> both rx_list and msg_ready).
>
> [this applies to all 3 RX functions, but this one is the simplest,
> which makes it more visible]
>
>> tls_rx_reader_unlock(sk, ctx);
>> return copied ? : err;
>>
>> @@ -2382,7 +2389,7 @@ int tls_sw_read_sock(struct sock *sk, read_descriptor_t *desc,
>> tlm = tls_msg(skb);
>> decrypted += rxm->full_len;
>>
>> - tls_rx_rec_done(ctx);
>> + tls_rx_rec_release(ctx);
>
> With this, there's no tls_strp_check_rcv() call inside the
> tls_sw_read_sock() loop, so in the next iteration, tls_rx_rec_wait()
> will have to go for at least one round of its own loop. [this points
> back to the recvmsg comment above, and a bit to the "cold path"
> discussion we had earlier]
All three points are really the same architectural question:
where should the parse-next-record step live -- eagerly inline
after release, or lazily in tls_rx_rec_wait()? And when a record
is already parsed at function exit, should the wakeup be
considered spurious or useful?
I’d like to try to help answer those questions. I set up loopback
NFS-over-TLS on a 10-CPU test system running these patches with
patches that replace recvmsg in NFSD with read_sock. I profiled
the server-side receive path with perf under several fio write
workloads. The backing store is tmpfs, so disk I/O is not a factor.
At 1M block size with a single fio job (540 IOPS, 540 MiB/s),
AES-GCM decryption dominates at 49% of cycles. The TLS record
parsing functions (tls_strp_check_rcv, tls_rx_rec_wait,
tls_strp_msg_release) do not appear above the 0.5% threshold.
At 4k block size with 8 jobs over a single connection (29K
IOPS, 113 MiB/s), AES-GCM drops to 5% and mutex contention
on the socket lock rises to 6.4%. tls_strp_check_rcv and
tls_rx_rec_wait each account for 0.05%.
At 4k block size with 32 jobs over nconnect=8 (87K IOPS,
339 MiB/s), mutex contention drops to 0.75% as the load
spreads across eight TLS connections. tls_strp_check_rcv
rises to 0.14% and tls_rx_rec_wait to 0.12%. The profile
is broadly distributed across NFS compound processing, XDR
encode/decode, credential handling, TCP transmit, and memory
allocation.
Across all three workloads the total cost of the functions
reorganized by patch 4/5 -- tls_strp_check_rcv,
tls_strp_msg_release, tls_rx_rec_wait, and tls_rx_msg_ready
-- stays below 0.5% of server CPU cycles. Moving the
tls_strp_check_rcv call site from its current position to
tls_rx_rec_wait, or adding an eager parse inside the
tls_sw_read_sock loop, would not produce a measurable
difference at these load levels.
From a performance standpoint, the architecture doesn’t
really matter. I think we have room to innovate when we
get around to async support for decryption (which I agree
will be challenging).
--
Chuck Lever
^ permalink raw reply
* Re: [PATCH net 2/3] bnxt_en: Don't assume XDP is never enabled in bnxt_init_dflt_ring_mode()
From: Jakub Kicinski @ 2026-03-31 2:05 UTC (permalink / raw)
To: Michael Chan
Cc: davem, netdev, edumazet, pabeni, andrew+netdev, pavan.chebbi,
andrew.gospodarek, Kalesh AP
In-Reply-To: <20260329232528.870637-3-michael.chan@broadcom.com>
On Sun, 29 Mar 2026 16:25:27 -0700 Michael Chan wrote:
> Fixes: 93550a9c7939 ("bnxt_en: implement devlink dev reload driver_reinit")
Fixes tag: Fixes: 93550a9c7939 ("bnxt_en: implement devlink dev reload driver_reinit")
Has these problem(s):
- Target SHA1 does not exist
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH net-next v4 3/3] r8152: add helper functions for PHY OCP registers
From: Jakub Kicinski @ 2026-03-31 2:01 UTC (permalink / raw)
To: hsu.chih.kai
Cc: Jakub Kicinski, davem, netdev, nic_swsd, linux-kernel, linux-usb,
edumazet, bjorn, pabeni
In-Reply-To: <20260326073925.32976-456-nic_swsd@realtek.com>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 32a4e8d423115..8747c55e0a484 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
[ ... ]
> +static void sram_write_w0w1(struct r8152 *tp, u16 addr, u16 clear, u16 set)
> +{
> + u16 data;
> +
> + data = sram_read(tp, addr);
> + data = (data & ~clear) | set;
> + ocp_reg_write(tp, OCP_SRAM_DATA, data);
AI flags:
Does writing directly to OCP_SRAM_DATA here actually write to the intended
address?
Because hardware indirect SRAM access ports typically auto-increment their
internal address pointer upon access, the sram_read() call will advance the
pointer. By writing directly to OCP_SRAM_DATA instead of using
sram_write(tp, addr, data) to re-establish the address, the updated value
would be written to the next address (addr + 1) rather than the original addr.
Could this corrupt the PHY configuration memory across the initialization paths
that now use sram_clr_bits() and sram_set_bits()?
Can you confirm if this is legit? Not sure whether this device / register
auto-increments or where AI got it from.
^ permalink raw reply
* Re: [PATCH net-next] FDDI: defza: Rate-limit memory allocation errors
From: patchwork-bot+netdevbpf @ 2026-03-31 2:00 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, netdev,
linux-kernel
In-Reply-To: <alpine.DEB.2.21.2603291252380.60268@angie.orcam.me.uk>
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Sun, 29 Mar 2026 13:32:34 +0100 (BST) you wrote:
> Prevent the system from becoming unstable or unusable due to a flood of
> memory allocation error messages under memory pressure.
>
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> ---
> drivers/net/fddi/defza.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> [...]
Here is the summary with links:
- [net-next] FDDI: defza: Rate-limit memory allocation errors
https://git.kernel.org/netdev/net-next/c/f4ef5b1c1331
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net-next] FDDI: defxx: Rate-limit memory allocation errors
From: patchwork-bot+netdevbpf @ 2026-03-31 2:00 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, netdev,
linux-kernel
In-Reply-To: <alpine.DEB.2.21.2603291236590.60268@angie.orcam.me.uk>
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Sun, 29 Mar 2026 13:32:25 +0100 (BST) you wrote:
> Prevent the system from becoming unstable or unusable due to a flood of
> memory allocation error messages under memory pressure, e.g.:
>
> [...]
> fddi0: Could not allocate receive buffer. Dropping packet.
> fddi0: Could not allocate receive buffer. Dropping packet.
> fddi0: Could not allocate receive buffer. Dropping packet.
> fddi0: Could not allocate receive buffer. Dropping packet.
> rcu: INFO: rcu_sched self-detected stall on CPU
> rcu: 0-...!: (332 ticks this GP) idle=255c/1/0x40000000 softirq=16420123/16420123 fqs=0
> rcu: (t=2103 jiffies g=35680089 q=4 ncpus=1)
> rcu: rcu_sched kthread timer wakeup didn't happen for 2102 jiffies! g35680089 f0x0 RCU_GP_WAIT_FQS(5) ->state=0x402
> rcu: Possible timer handling issue on cpu=0 timer-softirq=12779658
> rcu: rcu_sched kthread starved for 2103 jiffies! g35680089 f0x0 RCU_GP_WAIT_FQS(5) ->state=0x402 ->cpu=0
> rcu: Unless rcu_sched kthread gets sufficient CPU time, OOM is now expected behavior.
> rcu: RCU grace-period kthread stack dump:
> task:rcu_sched state:I stack:0 pid:14 tgid:14 ppid:2 flags:0x00004000
> Call Trace:
> __schedule+0x258/0x580
> schedule+0x19/0xa0
> schedule_timeout+0x4a/0xb0
> ? hrtimers_cpu_dying+0x1b0/0x1b0
> rcu_gp_fqs_loop+0xb1/0x450
> rcu_gp_kthread+0x9d/0x130
> kthread+0xb2/0xe0
> ? rcu_gp_init+0x4a0/0x4a0
> ? kthread_park+0x90/0x90
> ret_from_fork+0x2d/0x50
> ? kthread_park+0x90/0x90
> ret_from_fork_asm+0x12/0x20
> entry_INT80_32+0x10d/0x10d
> CPU: 0 UID: 500 PID: 21895 Comm: 31370.exe Not tainted 6.13.0-dirty #2
>
> [...]
Here is the summary with links:
- [net-next] FDDI: defxx: Rate-limit memory allocation errors
https://git.kernel.org/netdev/net-next/c/7fae6616704a
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [net-next,PATCH v5 3/3] net: phy: realtek: Add property to enable SSC
From: Jakub Kicinski @ 2026-03-31 1:57 UTC (permalink / raw)
To: Marek Vasut
Cc: netdev, David S. Miller, Aleksander Jan Bajkowski, Andrew Lunn,
Conor Dooley, Eric Dumazet, Florian Fainelli, Heiner Kallweit,
Ivan Galkin, Krzysztof Kozlowski, Michael Klein, Paolo Abeni,
Rob Herring, Russell King, Vladimir Oltean, devicetree
In-Reply-To: <20260326210704.58912-3-marek.vasut@mailbox.org>
On Thu, 26 Mar 2026 22:06:35 +0100 Marek Vasut wrote:
> +/* RTL8211F SSC settings */
> +#define RTL8211F_SSC_PAGE 0xc44
> +#define RTL8211F_SSC_RXC 0x13
> +#define RTL8211F_SSC_SYSCLK 0x17
> +#define RTL8211F_SSC_CLKOUT 0x19
> + /* Unnamed registers from EMI improvement parameters application note 1.2 */
> + ret = phy_write_paged(phydev, 0xd09, 0x10, 0xcf00);
> + if (ret < 0) {
> + dev_err(dev, "CLKOUT SSC initialization failed: %pe\n", ERR_PTR(ret));
> + return ret;
> + }
> +
> + ret = phy_write(phydev, RTL8211F_SSC_CLKOUT, 0x38c3);
> + if (ret < 0) {
> + dev_err(dev, "CLKOUT SSC configuration failed: %pe\n", ERR_PTR(ret));
> + return ret;
> + }
AI flags that this, did you mean to write to the SSC_PAGE here?
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH bpf v4 2/2] selftests/bpf: Add protocol check test for bpf_sk_assign_tcp_reqsk()
From: Kuniyuki Iwashima @ 2026-03-31 1:50 UTC (permalink / raw)
To: Jiayuan Chen
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Shuah Khan, netdev, linux-kernel, linux-kselftest
In-Reply-To: <20260330080746.319680-3-jiayuan.chen@linux.dev>
On Mon, Mar 30, 2026 at 1:08 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
>
> Add test_tcp_custom_syncookie_protocol_check to verify that
> bpf_sk_assign_tcp_reqsk() rejects non-TCP skbs. The test sends a UDP
> packet through TC ingress where a BPF program calls
> bpf_sk_assign_tcp_reqsk() on it and checks that the kfunc returns an
> error. A UDP server recv() is used as synchronization to ensure the
> BPF program has finished processing before checking the result.
>
> Without the fix in bpf_sk_assign_tcp_reqsk(), the kfunc succeeds and
> attaches a TCP reqsk to the UDP skb, which causes a null pointer
> dereference panic when the kernel processes it through the UDP receive
> path.
>
> Test result:
>
> ./test_progs -a tcp_custom_syncookie_protocol_check -v
> setup_netns:PASS:create netns 0 nsec
> setup_netns:PASS:ip 0 nsec
> write_sysctl:PASS:open sysctl 0 nsec
> write_sysctl:PASS:write sysctl 0 nsec
> setup_netns:PASS:write_sysctl 0 nsec
> test_tcp_custom_syncookie_protocol_check:PASS:open_and_load 0 nsec
> setup_tc:PASS:qdisc add dev lo clsact 0 nsec
> setup_tc:PASS:filter add dev lo ingress 0 nsec
> run_protocol_check:PASS:start tcp_server 0 nsec
> run_protocol_check:PASS:start udp_server 0 nsec
> run_protocol_check:PASS:connect udp_client 0 nsec
> run_protocol_check:PASS:send udp 0 nsec
> run_protocol_check:PASS:recv udp 0 nsec
> run_protocol_check:PASS:udp_intercepted 0 nsec
> run_protocol_check:PASS:assign_ret 0 nsec
> #471/1 tcp_custom_syncookie_protocol_check/IPv4 TCP:OK
> run_protocol_check:PASS:start tcp_server 0 nsec
> run_protocol_check:PASS:start udp_server 0 nsec
> run_protocol_check:PASS:connect udp_client 0 nsec
> run_protocol_check:PASS:send udp 0 nsec
> run_protocol_check:PASS:recv udp 0 nsec
> run_protocol_check:PASS:udp_intercepted 0 nsec
> run_protocol_check:PASS:assign_ret 0 nsec
> #471/2 tcp_custom_syncookie_protocol_check/IPv6 TCP:OK
> #471 tcp_custom_syncookie_protocol_check:OK
> Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
>
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> ---
> .../bpf/prog_tests/tcp_custom_syncookie.c | 91 ++++++++++++++-
> .../bpf/progs/test_tcp_custom_syncookie.c | 109 ++++++++++++++++++
> 2 files changed, 196 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_custom_syncookie.c b/tools/testing/selftests/bpf/prog_tests/tcp_custom_syncookie.c
> index eaf441dc7e79..6e29402c4c59 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tcp_custom_syncookie.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tcp_custom_syncookie.c
> @@ -5,6 +5,7 @@
> #include <sched.h>
> #include <stdlib.h>
> #include <net/if.h>
> +#include <netinet/in.h>
>
> #include "test_progs.h"
> #include "cgroup_helpers.h"
> @@ -47,11 +48,10 @@ static int setup_netns(void)
> return -1;
> }
>
> -static int setup_tc(struct test_tcp_custom_syncookie *skel)
> +static int setup_tc(int prog_fd)
> {
> LIBBPF_OPTS(bpf_tc_hook, qdisc_lo, .attach_point = BPF_TC_INGRESS);
> - LIBBPF_OPTS(bpf_tc_opts, tc_attach,
> - .prog_fd = bpf_program__fd(skel->progs.tcp_custom_syncookie));
> + LIBBPF_OPTS(bpf_tc_opts, tc_attach, .prog_fd = prog_fd);
>
> qdisc_lo.ifindex = if_nametoindex("lo");
> if (!ASSERT_OK(bpf_tc_hook_create(&qdisc_lo), "qdisc add dev lo clsact"))
> @@ -127,7 +127,7 @@ void test_tcp_custom_syncookie(void)
> if (!ASSERT_OK_PTR(skel, "open_and_load"))
> return;
>
> - if (setup_tc(skel))
> + if (setup_tc(bpf_program__fd(skel->progs.tcp_custom_syncookie)))
> goto destroy_skel;
>
> for (i = 0; i < ARRAY_SIZE(test_cases); i++) {
> @@ -145,6 +145,89 @@ void test_tcp_custom_syncookie(void)
>
> destroy_skel:
> system("tc qdisc del dev lo clsact");
> + test_tcp_custom_syncookie__destroy(skel);
> +}
> +
> +/* Test: bpf_sk_assign_tcp_reqsk() should reject non-TCP skb.
> + *
> + * Send a UDP packet through TC ingress where a BPF program calls
> + * bpf_sk_assign_tcp_reqsk() on it. The kfunc should return an error
> + * because the skb carries UDP, not TCP.
> + *
> + * TCP and UDP servers share the same port. The BPF program intercepts
> + * the UDP packet, looks up the TCP listener via the dest port, and
> + * attempts to assign a TCP reqsk to the UDP skb.
> + */
> +static void run_protocol_check(struct test_tcp_custom_syncookie *skel,
> + int family, const char *addr)
> +{
> + int tcp_server = -1, udp_server = -1, udp_client = -1;
nit: no need to init.
> + char buf[32] = "test";
should be buf[] = "test" since you send data of sizeof(buf) below.
> + int port, ret;
> +
> + tcp_server = start_server(family, SOCK_STREAM, addr, 0, 0);
> + if (!ASSERT_NEQ(tcp_server, -1, "start tcp_server"))
> + return;
> +
> + port = ntohs(get_socket_local_port(tcp_server));
> +
> + /* UDP server on same port for synchronization and port sharing */
> + udp_server = start_server(family, SOCK_DGRAM, addr, port, 0);
> + if (!ASSERT_NEQ(udp_server, -1, "start udp_server"))
> + goto close_tcp;
> +
> + skel->bss->udp_intercepted = false;
> + skel->bss->assign_ret = 0;
> +
> + udp_client = connect_to_fd(udp_server, 0);
> + if (!ASSERT_NEQ(udp_client, -1, "connect udp_client"))
> + goto close_udp_server;
>
> + ret = send(udp_client, buf, sizeof(buf), 0);
> + if (!ASSERT_EQ(ret, sizeof(buf), "send udp"))
> + goto close_udp_client;
memset(buf, 0, sizeof(buf)) here and
> +
> + /* recv() ensures TC ingress BPF has processed the skb */
> + ret = recv(udp_server, buf, sizeof(buf), 0);
> + if (!ASSERT_EQ(ret, sizeof(buf), "recv udp"))
check ASSERT_STREQ() here ?
> + goto close_udp_client;
> +
> + ASSERT_EQ(skel->bss->udp_intercepted, true, "udp_intercepted");
> +
> + ASSERT_EQ(skel->bss->assign_ret, -EINVAL, "assign_ret");
> +
> +close_udp_client:
> + close(udp_client);
> +close_udp_server:
> + close(udp_server);
> +close_tcp:
> + close(tcp_server);
> +}
> +
> +void test_tcp_custom_syncookie_protocol_check(void)
> +{
> + struct test_tcp_custom_syncookie *skel;
> + int i;
> +
> + if (setup_netns())
> + return;
> +
> + skel = test_tcp_custom_syncookie__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "open_and_load"))
> + return;
> +
> + if (setup_tc(bpf_program__fd(skel->progs.tcp_custom_syncookie_badproto)))
> + goto destroy_skel;
> +
> + for (i = 0; i < ARRAY_SIZE(test_cases); i++) {
> + if (!test__start_subtest(test_cases[i].name))
> + continue;
> +
> + run_protocol_check(skel, test_cases[i].family,
> + test_cases[i].addr);
> + }
> +
> +destroy_skel:
> + system("tc qdisc del dev lo clsact");
> test_tcp_custom_syncookie__destroy(skel);
> }
> diff --git a/tools/testing/selftests/bpf/progs/test_tcp_custom_syncookie.c b/tools/testing/selftests/bpf/progs/test_tcp_custom_syncookie.c
> index 7d5293de1952..bd3fad3dd503 100644
> --- a/tools/testing/selftests/bpf/progs/test_tcp_custom_syncookie.c
> +++ b/tools/testing/selftests/bpf/progs/test_tcp_custom_syncookie.c
> @@ -588,4 +588,113 @@ int tcp_custom_syncookie(struct __sk_buff *skb)
> return tcp_handle_ack(&ctx);
> }
>
> +/* Test: call bpf_sk_assign_tcp_reqsk() on a UDP skb.
> + * The kfunc should reject it because the skb is not TCP.
> + *
> + * TCP and UDP servers share the same port. The BPF program intercepts
> + * UDP packets, looks up the TCP listener on the same port, and tries
> + * to assign a TCP reqsk to the UDP skb.
> + */
> +int assign_ret = 0;
> +bool udp_intercepted = false;
No need to init var on bss w/ 0 here.
(init in run_protocol_check() is necessary)
> +
> +static int badproto_lookup_assign(struct __sk_buff *skb, struct udphdr *udp,
> + struct bpf_sock_tuple *tuple, u32 tuple_size)
> +{
> + struct bpf_tcp_req_attrs attrs = {};
> + struct bpf_sock *skc;
> + struct sock *sk;
> +
> + skc = bpf_skc_lookup_tcp(skb, tuple, tuple_size, -1, 0);
> + if (!skc)
> + return TC_ACT_OK;
> +
> + if (skc->state != TCP_LISTEN) {
> + bpf_sk_release(skc);
> + return TC_ACT_OK;
> + }
> +
> + sk = (struct sock *)bpf_skc_to_tcp_sock(skc);
> + if (!sk) {
> + bpf_sk_release(skc);
> + return TC_ACT_OK;
> + }
> +
> + attrs.mss = 1460;
> + attrs.wscale_ok = 1;
> + attrs.snd_wscale = 7;
> + attrs.rcv_wscale = 7;
> + attrs.sack_ok = 1;
> +
> + assign_ret = bpf_sk_assign_tcp_reqsk(skb, sk, &attrs, sizeof(attrs));
> +
> + bpf_sk_release(skc);
> + return TC_ACT_OK;
> +}
> +
> +SEC("tc")
> +int tcp_custom_syncookie_badproto(struct __sk_buff *skb)
> +{
> + void *data = (void *)(long)skb->data;
> + void *data_end = (void *)(long)skb->data_end;
> + struct bpf_sock_tuple tuple = {};
> + struct ethhdr *eth;
> + struct iphdr *iph;
> + struct ipv6hdr *ip6h;
> + struct udphdr *udp;
> +
> + eth = (struct ethhdr *)data;
> + if (eth + 1 > data_end)
> + return TC_ACT_OK;
> +
> + switch (bpf_ntohs(eth->h_proto)) {
> + case ETH_P_IP:
> + iph = (struct iphdr *)(eth + 1);
> + if (iph + 1 > data_end)
> + return TC_ACT_OK;
> +
> + if (iph->protocol != IPPROTO_UDP)
> + return TC_ACT_OK;
> +
> + udp = (struct udphdr *)(iph + 1);
> + if (udp + 1 > data_end)
> + return TC_ACT_OK;
> +
> + udp_intercepted = true;
> +
> + tuple.ipv4.saddr = iph->saddr;
> + tuple.ipv4.daddr = iph->daddr;
> + tuple.ipv4.sport = udp->source;
> + tuple.ipv4.dport = udp->dest;
> +
> + return badproto_lookup_assign(skb, udp, &tuple,
> + sizeof(tuple.ipv4));
> + case ETH_P_IPV6:
> + ip6h = (struct ipv6hdr *)(eth + 1);
> + if (ip6h + 1 > data_end)
> + return TC_ACT_OK;
> +
> + if (ip6h->nexthdr != IPPROTO_UDP)
> + return TC_ACT_OK;
> +
> + udp = (struct udphdr *)(ip6h + 1);
> + if (udp + 1 > data_end)
> + return TC_ACT_OK;
> +
> + udp_intercepted = true;
> +
> + __builtin_memcpy(tuple.ipv6.saddr, &ip6h->saddr,
> + sizeof(tuple.ipv6.saddr));
> + __builtin_memcpy(tuple.ipv6.daddr, &ip6h->daddr,
> + sizeof(tuple.ipv6.daddr));
> + tuple.ipv6.sport = udp->source;
> + tuple.ipv6.dport = udp->dest;
> +
> + return badproto_lookup_assign(skb, udp, &tuple,
> + sizeof(tuple.ipv6));
> + default:
> + return TC_ACT_OK;
> + }
> +}
> +
> char _license[] SEC("license") = "GPL";
> --
> 2.43.0
>
^ permalink raw reply
* [PATCH] net: alteon: Add missing DMA mapping error checks in ace_start_xmit
From: Wang Jun @ 2026-03-31 1:48 UTC (permalink / raw)
To: Jes Sorensen, Andrew Lunn, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Cc: linux-acenic, netdev, linux-kernel, gszhai, 25125332, 25125283,
23120469, Wang Jun, stable
The ace_start_xmit function does not check the return value of
dma_map_page (via ace_map_tx_skb) and skb_frag_dma_map when building
transmit descriptors. If mapping fails, an invalid DMA address is
written to the descriptor, which may cause hardware to access
illegal memory, leading to system instability or crashes.
Add proper dma_mapping_error() checks for all mapping calls. When
mapping fails, free the skb, increment the dropped packet counter,
and return NETDEV_TX_OK.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Wang Jun <1742789905@qq.com>
---
drivers/net/ethernet/alteon/acenic.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/net/ethernet/alteon/acenic.c b/drivers/net/ethernet/alteon/acenic.c
index 455ee8930824..acabede53663 100644
--- a/drivers/net/ethernet/alteon/acenic.c
+++ b/drivers/net/ethernet/alteon/acenic.c
@@ -2417,6 +2417,11 @@ static netdev_tx_t ace_start_xmit(struct sk_buff *skb,
u32 vlan_tag = 0;
mapping = ace_map_tx_skb(ap, skb, skb, idx);
+ if (dma_mapping_error(&ap->pdev->dev, mapping)) {
+ dev_kfree_skb(skb);
+ dev->stats.tx_dropped++;
+ return NETDEV_TX_OK;
+ }
flagsize = (skb->len << 16) | (BD_FLG_END);
if (skb->ip_summed == CHECKSUM_PARTIAL)
flagsize |= BD_FLG_TCP_UDP_SUM;
@@ -2438,6 +2443,11 @@ static netdev_tx_t ace_start_xmit(struct sk_buff *skb,
int i;
mapping = ace_map_tx_skb(ap, skb, NULL, idx);
+ if (dma_mapping_error(&ap->pdev->dev, mapping)) {
+ dev_kfree_skb(skb);
+ dev->stats.tx_dropped++;
+ return NETDEV_TX_OK;
+ }
flagsize = (skb_headlen(skb) << 16);
if (skb->ip_summed == CHECKSUM_PARTIAL)
flagsize |= BD_FLG_TCP_UDP_SUM;
@@ -2460,6 +2470,11 @@ static netdev_tx_t ace_start_xmit(struct sk_buff *skb,
mapping = skb_frag_dma_map(&ap->pdev->dev, frag, 0,
skb_frag_size(frag),
DMA_TO_DEVICE);
+ if (dma_mapping_error(&ap->pdev->dev, mapping)) {
+ dev_kfree_skb(skb);
+ dev->stats.tx_dropped++;
+ return NETDEV_TX_OK;
+ }
flagsize = skb_frag_size(frag) << 16;
if (skb->ip_summed == CHECKSUM_PARTIAL)
--
2.43.0
^ permalink raw reply related
* Re: [PATCH bpf v4 1/2] bpf: tcp: Reject non-TCP skb in bpf_sk_assign_tcp_reqsk()
From: Kuniyuki Iwashima @ 2026-03-31 1:35 UTC (permalink / raw)
To: Jiayuan Chen
Cc: bot+bpf-ci, bpf, clm, martin.lau, daniel, john.fastabend, sdf,
ast, andrii, eddyz87, song, yonghong.song, kpsingh, haoluo, jolsa,
davem, edumazet, kuba, pabeni, horms, shuah, netdev, linux-kernel,
linux-kselftest, martin.lau, ihor.solodrai
In-Reply-To: <d1a739d2-c4ea-4fae-aeaf-f49e75dac0f3@linux.dev>
On Mon, Mar 30, 2026 at 2:26 AM Jiayuan Chen <jiayuan.chen@linux.dev> wrote:
>
>
> On 3/30/26 5:00 PM, bot+bpf-ci@kernel.org wrote:
> >> #if IS_BUILTIN(CONFIG_IPV6)
> >> case htons(ETH_P_IPV6):
> >> + if (ipv6_hdr(skb)->nexthdr != IPPROTO_TCP)
> >> + return -EINVAL;
> >> +
> >> ops = &tcp6_request_sock_ops;
> > For IPv6, ipv6_hdr(skb)->nexthdr gives the type of the header
> > immediately following the base IPv6 header. If extension headers
> > are present (hop-by-hop options, routing, etc.), nexthdr would be
> > the extension header type rather than IPPROTO_TCP, even when the
> > packet is a valid TCP segment.
> >
> > Would it be worth using ipv6_find_hdr() here, similar to
> > bpf_update_srh_state() in the same file, to walk past any extension
> > headers? The IPv4 check above is fine since ip_hdr(skb)->protocol
> > always identifies the transport protocol directly.
> >
> > In practice this is very unlikely to matter since TCP SYN packets
> > with IPv6 extension headers are essentially non-existent, but the
> > check as written would reject them.
> >
> >
> > ---
> > AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> > See:https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> >
> > CI run summary:https://github.com/kernel-patches/bpf/actions/runs/23735111188
>
>
> Thanks for the analysis.
>
> There are many places in the kernel that check nexthdr directly without
> walking extension headers.
>
> I'd prefer to keep it simple for now and leave ipv6_find_hdr() as a
> potential future improvement if needed.
+1.
Given this feature is to create a reqsk to process on this running
host, it does not make sense to support such ext options.
^ permalink raw reply
* Re: [PATCH net v2] ipv6: fix data race in fib6_metric_set() using cmpxchg
From: Hangbin Liu @ 2026-03-31 1:34 UTC (permalink / raw)
To: Jiayuan Chen
Cc: David S. Miller, David Ahern, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Jiayuan Chen, David Ahern, netdev,
linux-kernel, Fei Liu
In-Reply-To: <7e8c45cb-b5dd-4694-80e9-7c8e5ad9ade7@163.com>
On Sat, Mar 28, 2026 at 07:22:48PM +0800, Jiayuan Chen wrote:
> https://sashiko.dev/#/patchset/20260327-b4-fib6_metric_set-kmemleak-v2-1-366b2c78b5c2%40gmail.com
>
>
> The concern about reader paths (e.g., ip_dst_init_metrics, fib6_pmtu)
> lacking READ_ONCE()
> annotations is valid — if the compiler reloads from->fib6_metrics after
> inlining, it could produce
> an inconsistent pointer/flags combination in dst->_metrics, potentially
> leading to a refcount_dec
> on the read-only dst_default_metrics.
Thanks, I will fix the reader path separately in case I missed anything and
slow down this one's process.
Thanks
Hangbin
^ permalink raw reply
* Re: [PATCH net-next v3 1/4] udp: Only compare daddr/dport when sk_state == TCP_ESTABLISHED
From: Kuniyuki Iwashima @ 2026-03-31 1:21 UTC (permalink / raw)
To: Jordan Rife
Cc: netdev, bpf, Willem de Bruijn, Eric Dumazet, Daniel Borkmann,
Martin KaFai Lau, Stanislav Fomichev, Andrii Nakryiko,
Yusuke Suzuki, Jakub Kicinski
In-Reply-To: <20260330215707.2374657-2-jrife@google.com>
On Mon, Mar 30, 2026 at 2:57 PM Jordan Rife <jrife@google.com> wrote:
>
> Adjust lookups and scoring to keep their results equivalent to before
> even if inet_daddr+inet_dport are left intact after disconnecting a
> socket (sk_state == TCP_CLOSE). sk_state == TCP_ESTABLISHED implies that
> *daddr is non-zero, so remove redundant checks for that at the same
> time. Note that __udp6_lib_demux_lookup already checks if sk_state ==
> TCP_ESTABLISHED, so no change was needed there [1].
>
> I could find no discernible difference in performance in
> udp4_lib_lookup2 before and after the change in compute_score.
What workload did you test the series with ?
I think we want to see results under DDoS.
>
> (AMD Ryzen 9 9900X)
>
> kprobe:udp4_lib_lookup2 {
> @start[cpu] = nsecs;
> }
> kretprobe:udp4_lib_lookup2 {
> @lookup[cpu] = hist(nsecs - @start[cpu], 2);
> }
>
> BEFORE
> ======
> @lookup[11]:
> [80, 96) 1387077 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> [96, 112) 364973 |@@@@@@@@@@@@@ |
> [112, 128) 34261 |@ |
> [128, 160) 7246 | |
> [160, 192) 215 | |
> [192, 224) 126 | |
>
> AFTER
> =====
> @lookup[11]:
> [80, 96) 1408594 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> [96, 112) 340568 |@@@@@@@@@@@@ |
> [112, 128) 30753 |@ |
> [128, 160) 8019 | |
> [160, 192) 231 | |
> [192, 224) 157 | |
>
> [1]: https://lore.kernel.org/netdev/20170623222537.130493-1-tracywwnj@gmail.com/
>
> Signed-off-by: Jordan Rife <jrife@google.com>
> ---
> net/ipv4/udp.c | 20 +++++++++++---------
> net/ipv6/udp.c | 18 +++++++++---------
> 2 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index b60fad393e18..d91c587c3657 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -385,16 +385,16 @@ static int compute_score(struct sock *sk, const struct net *net,
> score = (sk->sk_family == PF_INET) ? 2 : 1;
>
> inet = inet_sk(sk);
> - if (inet->inet_daddr) {
> + if (sk->sk_state == TCP_ESTABLISHED) {
> if (inet->inet_daddr != saddr)
> return -1;
> score += 4;
> - }
>
> - if (inet->inet_dport) {
> - if (inet->inet_dport != sport)
> - return -1;
> - score += 4;
> + if (inet->inet_dport) {
> + if (inet->inet_dport != sport)
> + return -1;
> + score += 4;
> + }
> }
>
> dev_match = udp_sk_bound_dev_eq(net, sk->sk_bound_dev_if,
> @@ -796,8 +796,9 @@ static inline bool __udp_is_mcast_sock(struct net *net, const struct sock *sk,
>
> if (!net_eq(sock_net(sk), net) ||
> udp_sk(sk)->udp_port_hash != hnum ||
> - (inet->inet_daddr && inet->inet_daddr != rmt_addr) ||
> - (inet->inet_dport != rmt_port && inet->inet_dport) ||
> + (sk->sk_state == TCP_ESTABLISHED &&
> + (inet->inet_daddr != rmt_addr ||
> + (inet->inet_dport != rmt_port && inet->inet_dport))) ||
> (inet->inet_rcv_saddr && inet->inet_rcv_saddr != loc_addr) ||
> ipv6_only_sock(sk) ||
> !udp_sk_bound_dev_eq(net, sk->sk_bound_dev_if, dif, sdif))
> @@ -2854,7 +2855,8 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
> ports = INET_COMBINED_PORTS(rmt_port, hnum);
>
> udp_portaddr_for_each_entry_rcu(sk, &hslot2->head) {
> - if (inet_match(net, sk, acookie, ports, dif, sdif))
> + if (sk->sk_state == TCP_ESTABLISHED &&
> + inet_match(net, sk, acookie, ports, dif, sdif))
> return sk;
> /* Only check first socket in chain */
> break;
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 010b909275dd..b93a9a3e7678 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -147,16 +147,16 @@ static int compute_score(struct sock *sk, const struct net *net,
> score = 0;
> inet = inet_sk(sk);
>
> - if (inet->inet_dport) {
> + if (sk->sk_state == TCP_ESTABLISHED) {
> if (inet->inet_dport != sport)
> return -1;
> score++;
> - }
>
> - if (!ipv6_addr_any(&sk->sk_v6_daddr)) {
> - if (!ipv6_addr_equal(&sk->sk_v6_daddr, saddr))
> - return -1;
> - score++;
> + if (!ipv6_addr_any(&sk->sk_v6_daddr)) {
This looks unnecessary.
> + if (!ipv6_addr_equal(&sk->sk_v6_daddr, saddr))
> + return -1;
> + score++;
> + }
> }
>
> bound_dev_if = READ_ONCE(sk->sk_bound_dev_if);
> @@ -949,9 +949,9 @@ static bool __udp_v6_is_mcast_sock(struct net *net, const struct sock *sk,
>
> if (udp_sk(sk)->udp_port_hash != hnum ||
> sk->sk_family != PF_INET6 ||
> - (inet->inet_dport && inet->inet_dport != rmt_port) ||
> - (!ipv6_addr_any(&sk->sk_v6_daddr) &&
> - !ipv6_addr_equal(&sk->sk_v6_daddr, rmt_addr)) ||
> + (sk->sk_state == TCP_ESTABLISHED &&
> + ((inet->inet_dport && inet->inet_dport != rmt_port) ||
> + !ipv6_addr_equal(&sk->sk_v6_daddr, rmt_addr))) ||
> !udp_sk_bound_dev_eq(net, READ_ONCE(sk->sk_bound_dev_if), dif, sdif) ||
> (!ipv6_addr_any(&sk->sk_v6_rcv_saddr) &&
> !ipv6_addr_equal(&sk->sk_v6_rcv_saddr, loc_addr)))
> --
> 2.53.0.1118.gaef5881109-goog
>
^ permalink raw reply
* Re: [PATCH] net-shapers: free rollback entries using kfree_rcu
From: Jakub Kicinski @ 2026-03-31 1:15 UTC (permalink / raw)
To: Kangzheng Gu
Cc: gregkh, davem, edumazet, pabeni, horms, kees, p, netdev, stable,
linux-kernel
In-Reply-To: <20260328185804.41325-1-xiaoguai0992@gmail.com>
On Sat, 28 Mar 2026 18:58:04 +0000 Kangzheng Gu wrote:
> net_shaper_rollback() removes NET_SHAPER_NOT_VALID entries and frees
> them using kfree(), which can race with net_shaper_nl_get_dumpit() and
> lead to a use-after-free in net_shaper_fill_one().
>
> Use kfree_rcu() instead of kfree() to free rollback entries, since
> net_shaper_nl_get_dumpit() protects shaper access with rcu_read_lock().
If dump can see NOT_VALID entries we have a bigger problem than a UAF
don't you think? :/
^ permalink raw reply
* Re: [PATCH net v2] ipv6: fix data race in fib6_metric_set() using cmpxchg
From: Hangbin Liu @ 2026-03-31 1:12 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David S. Miller, David Ahern, Eric Dumazet, Paolo Abeni,
Simon Horman, Jiayuan Chen, David Ahern, netdev, linux-kernel,
Fei Liu
In-Reply-To: <20260330174628.0cb1ebc6@kernel.org>
On Mon, Mar 30, 2026 at 05:46:28PM -0700, Jakub Kicinski wrote:
> On Fri, 27 Mar 2026 10:24:47 +0800 Hangbin Liu wrote:
> > --- a/net/ipv6/ip6_fib.c
> > +++ b/net/ipv6/ip6_fib.c
> > @@ -730,17 +730,24 @@ void fib6_metric_set(struct fib6_info *f6i, int metric, u32 val)
> > if (!f6i)
> > return;
> >
> > - if (f6i->fib6_metrics == &dst_default_metrics) {
> > + if (READ_ONCE(f6i->fib6_metrics) == &dst_default_metrics) {
> > + struct dst_metrics *dflt = (struct dst_metrics *)&dst_default_metrics;
>
> Why does this exist? To cast away the const?
Yes, cause cmpxchg doesn't accept const type.
>
> > struct dst_metrics *p = kzalloc_obj(*p, GFP_ATOMIC);
> >
> > if (!p)
> > return;
> >
> > + p->metrics[metric - 1] = val;
> > refcount_set(&p->refcnt, 1);
> > - f6i->fib6_metrics = p;
> > + if (cmpxchg(&f6i->fib6_metrics, dflt, p) != dflt)
> > + kfree(p);
> > + else
> > + return;
> > }
> >
> > - f6i->fib6_metrics->metrics[metric - 1] = val;
> > + struct dst_metrics *m = READ_ONCE(f6i->fib6_metrics);
>
> No variable declarations in the middle of a function please.
Oh, I thought it's OK now since kernel supports C99...
I will fix it.
Thanks
Hangbin
^ permalink raw reply
* Re: [PATCH net-next v2] net: airoha: Delay offloading until all net_devices are fully registered
From: patchwork-bot+netdevbpf @ 2026-03-31 1:10 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, linux-arm-kernel,
linux-mediatek, netdev
In-Reply-To: <20260329-airoha-regiser-race-fix-v2-1-f4ebb139277b@kernel.org>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Sun, 29 Mar 2026 12:32:27 +0200 you wrote:
> Netfilter flowtable can theoretically try to offload flower rules as soon
> as a net_device is registered while all the other ones are not
> registered or initialized, triggering a possible NULL pointer dereferencing
> of qdma pointer in airoha_ppe_set_cpu_port routine. Moreover, if
> register_netdev() fails for a particular net_device, there is a small
> race if Netfilter tries to offload flowtable rules before all the
> net_devices are properly unregistered in airoha_probe() error patch,
> triggering a NULL pointer dereferencing in airoha_ppe_set_cpu_port
> routine. In order to avoid any possible race, delay offloading until
> all net_devices are registered in the networking subsystem.
>
> [...]
Here is the summary with links:
- [net-next,v2] net: airoha: Delay offloading until all net_devices are fully registered
https://git.kernel.org/netdev/net/c/cedc1bf327de
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net v7] bnxt_en: set backing store type from query type
From: patchwork-bot+netdevbpf @ 2026-03-31 1:10 UTC (permalink / raw)
To: Pengpeng Hou
Cc: michael.chan, pavan.chebbi, andrew+netdev, davem, edumazet, kuba,
pabeni, netdev, linux-kernel
In-Reply-To: <20260328234357.43669-1-pengpeng@iscas.ac.cn>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Sun, 29 Mar 2026 07:43:56 +0800 you wrote:
> bnxt_hwrm_func_backing_store_qcaps_v2() stores resp->type from the
> firmware response in ctxm->type and later uses that value to index
> fixed backing-store metadata arrays such as ctx_arr[] and
> bnxt_bstore_to_trace[].
>
> ctxm->type is fixed by the current backing-store query type and matches
> the array index of ctx->ctx_arr. Set ctxm->type from the current loop
> variable instead of depending on resp->type.
>
> [...]
Here is the summary with links:
- [net,v7] bnxt_en: set backing store type from query type
https://git.kernel.org/netdev/net/c/4ee937107d52
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: mxl862xx: cancel pending work on probe error
From: Andrew Lunn @ 2026-03-31 1:07 UTC (permalink / raw)
To: Daniel Golle
Cc: Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-kernel
In-Reply-To: <3fd163f5bb88de426ca9847549f94b4296170ef0.1774911025.git.daniel@makrotopia.org>
On Mon, Mar 30, 2026 at 11:52:09PM +0100, Daniel Golle wrote:
> Call mxl862xx_host_shutdown() in case dsa_register_switch() returns
> an error, so any still pending crc_err_work get canceled.
>
> Fixes: a319d0c8c8ce ("net: dsa: mxl862xx: add CRC for MDIO communication")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v2 1/2] net: phy: microchip: add downshift tunable support for LAN88xx
From: Andrew Lunn @ 2026-03-31 1:02 UTC (permalink / raw)
To: Nicolai Buchwitz
Cc: netdev, Phil Elwell, Heiner Kallweit, Russell King,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel
In-Reply-To: <20260330224630.579937-2-nb@tipi-net.de>
On Tue, Mar 31, 2026 at 12:46:26AM +0200, Nicolai Buchwitz wrote:
> Implement the standard ETHTOOL_PHY_DOWNSHIFT tunable for the LAN88xx
> PHY. This allows runtime configuration of the auto-downshift feature
> via ethtool:
>
> ethtool --set-phy-tunable eth0 downshift on count 3
>
> The LAN88xx PHY supports downshifting from 1000BASE-T to 100BASE-TX
> after 2-5 failed auto-negotiation attempts. Valid count values are
> 2, 3, 4 and 5.
>
> This is based on an earlier downstream implementation by Phil Elwell.
>
> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [RFC PATCH net-next 2/3] seg6: add SRv6 L2 tunnel device (srl2)
From: Andrea Mayer @ 2026-03-31 0:59 UTC (permalink / raw)
To: nicolas.dichtel
Cc: netdev, David S . Miller, David Ahern, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Stefano Salsano,
Paolo Lungaroni, Ahmed Abdelsalam, Justin Iurman, linux-kernel,
Andrea Mayer
In-Reply-To: <a4a4ba6d-da13-4bb1-b6b2-1f9a5de190a7@6wind.com>
On Thu, 26 Mar 2026 17:44:29 +0100
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>
>
Thanks Nicolas for your time and for the review.
> Le 22/03/2026 à 01:05, Andrea Mayer a écrit :
> > Introduce srl2, an Ethernet pseudowire device over SRv6. It
> > encapsulates L2 frames in IPv6 with a Segment Routing Header for
> > transmission across an SRv6 network.
> >
> > The encapsulation logic reuses seg6_do_srh_encap() with
> > IPPROTO_ETHERNET. The transmit path uses the standard IPv6 tunnel
> > infrastructure (dst_cache, ip6_route_output, ip6tunnel_xmit).
> >
> > The device is configured with a segment list for point-to-point
> > L2 encapsulation.
> >
> > Usage:
> >
> > ip link add srl2-0 type srl2 segs fc00::a,fc00::b
> >
> > Co-developed-by: Stefano Salsano <stefano.salsano@uniroma2.it>
> > Signed-off-by: Stefano Salsano <stefano.salsano@uniroma2.it>
> > Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it>
> > ---
> > include/linux/srl2.h | 7 +
> > include/uapi/linux/srl2.h | 20 +++
> > net/ipv6/Kconfig | 16 +++
> > net/ipv6/Makefile | 1 +
> > net/ipv6/seg6.c | 1 +
> > net/ipv6/srl2.c | 269 ++++++++++++++++++++++++++++++++++++++
> > 6 files changed, 314 insertions(+)
> > create mode 100644 include/linux/srl2.h
> > create mode 100644 include/uapi/linux/srl2.h
> > create mode 100644 net/ipv6/srl2.c
> >
> > diff --git a/include/linux/srl2.h b/include/linux/srl2.h
> > new file mode 100644
> > index 000000000000..c1342b979402
> > --- /dev/null
> > +++ b/include/linux/srl2.h
> > @@ -0,0 +1,7 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +#ifndef _LINUX_SRL2_H
> > +#define _LINUX_SRL2_H
> > +
> > +#include <uapi/linux/srl2.h>
> > +
> > +#endif
> Is this really needed?
No. Moving IFLA_SRL2_* to if_link.h makes both srl2.h headers
unnecessary.
>
> > diff --git a/include/uapi/linux/srl2.h b/include/uapi/linux/srl2.h
> > new file mode 100644
> > index 000000000000..e7c8f6fc0791
> > --- /dev/null
> > +++ b/include/uapi/linux/srl2.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> > +/*
> > + * SRv6 L2 tunnel device
> > + *
> > + * Author:
> > + * Andrea Mayer <andrea.mayer@uniroma2.it>
> > + */
> > +
> > +#ifndef _UAPI_LINUX_SRL2_H
> > +#define _UAPI_LINUX_SRL2_H
> > +
> > +enum {
> > + IFLA_SRL2_UNSPEC,
> > + IFLA_SRL2_SRH, /* binary: struct ipv6_sr_hdr + segments */
> > + __IFLA_SRL2_MAX,
> > +};
> > +
> > +#define IFLA_SRL2_MAX (__IFLA_SRL2_MAX - 1)
> It should probably be generated automatically from specs, see
> https://docs.kernel.org/userspace-api/netlink/intro-specs.html
>
We'll add srl2 to rt-link.yaml. It seems that if_link.h is not
auto-generated from the spec yet, so I think we need to add
IFLA_SRL2_* there manually like the other link types.
> > [snip]
> > +static void srl2_setup(struct net_device *dev)
> > +{
> > + ether_setup(dev);
> > +
> > + dev->netdev_ops = &srl2_netdev_ops;
> > + dev->needs_free_netdev = true;
> > + dev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
> > + dev->needed_headroom = LL_MAX_HEADER + sizeof(struct ipv6hdr) +
> > + SRL2_SRH_HEADROOM_EST;
> > +
> > + dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> > + dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE;
> > + dev->lltx = true;
> > +
> Maybe setting dev->netns_immutable to true ?
>
It makes sense; the SRv6 configuration is bound to the current
netns, so the device should stay there as well.
> Regards,
> Nicolas
Thanks,
Andrea
^ permalink raw reply
* Re: [PATCH] net: sched: cls_api: fix tc_chain_fill_node to initialize tcm_info to zero to prevent an info-leak
From: patchwork-bot+netdevbpf @ 2026-03-31 1:00 UTC (permalink / raw)
To: Yochai E; +Cc: davem, jhs, jiri, security, netdev
In-Reply-To: <20260328211436.1010152-1-echelonh@gmail.com>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Sun, 29 Mar 2026 00:14:36 +0300 you wrote:
> When building netlink messages, tc_chain_fill_node() never initializes
> the tcm_info field of struct tcmsg. Since the allocation is not zeroed,
> kernel heap memory is leaked to userspace through this 4-byte field.
>
> The fix simply zeroes tcm_info alongside the other fields that are
> already initialized.
>
> [...]
Here is the summary with links:
- net: sched: cls_api: fix tc_chain_fill_node to initialize tcm_info to zero to prevent an info-leak
https://git.kernel.org/netdev/net/c/e6e3eb5ee89a
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [PATCH net-next v6 11/11] net: wangxun: implement pci_error_handlers ops
From: Jakub Kicinski @ 2026-03-31 0:55 UTC (permalink / raw)
To: jiawenwu
Cc: Jakub Kicinski, netdev, mengyuanlou, andrew+netdev, davem,
edumazet, pabeni, richardcochran, linux, horms,
michal.swiatkowski, jacob.e.keller, kees, joe, larysa.zaremba,
abdun.nihaal, leitao
In-Reply-To: <20260326021406.30444-12-jiawenwu@trustnetic.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wangxun: implement pci_error_handlers ops
Support AER driver to handle the PCIe errors for Wangxun devices.
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> index bb77bdce69d22..86b318647a3ca 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_err.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c
[ ... ]
> @@ -3,11 +3,113 @@
>
> #include <linux/netdevice.h>
> #include <linux/pci.h>
> +#include <linux/aer.h>
>
> #include "wx_type.h"
> #include "wx_lib.h"
> #include "wx_err.h"
[ ... ]
> +static pci_ers_result_t wx_io_slot_reset(struct pci_dev *pdev)
> +{
> + struct wx *wx = pci_get_drvdata(pdev);
> + pci_ers_result_t result;
> +
> + if (pci_enable_device_mem(pdev)) {
> + wx_err(wx, "Cannot re-enable PCI device after reset.\n");
> + result = PCI_ERS_RESULT_DISCONNECT;
> + } else {
> + /* make all bar access done before reset. */
> + smp_mb__before_atomic();
> + clear_bit(WX_STATE_DISABLED, wx->state);
> + pci_set_master(pdev);
> + pci_restore_state(pdev);
> + pci_save_state(pdev);
> + pci_wake_from_d3(pdev, false);
> +
> + wx->do_reset(wx->netdev, false);
Is it possible that the ngbe driver ignores this hardware reset request?
If ngbe_do_reset() only performs an action when reinit is true, it appears it
would be a no-op when called with false from this AER recovery path. Will
this leave the ngbe MAC hardware uninitialized after the PCI slot reset and
break the device when traffic resumes?
> + result = PCI_ERS_RESULT_RECOVERED;
> + }
> +
> + pci_aer_clear_nonfatal_status(pdev);
> +
> + return result;
> +}
> +
> +/**
> + * wx_io_resume - called when traffic can start flowing again.
> + * @pdev: Pointer to PCI device
> + *
> + * This callback is called when the error recovery driver tells us that
> + * its OK to resume normal operation.
> + */
> +static void wx_io_resume(struct pci_dev *pdev)
> +{
> + struct wx *wx = pci_get_drvdata(pdev);
> + struct net_device *netdev;
> +
> + netdev = wx->netdev;
> + rtnl_lock();
> + if (netif_running(netdev))
> + netdev->netdev_ops->ndo_open(netdev);
Does this code need to check the return value of ndo_open()?
If ndo_open() fails due to memory allocation or IRQ request failures, the
resources will remain unallocated. Because the failure is ignored and
netif_device_attach() is called unconditionally below, will the networking
stack invoke the transmit handler and dereference the unallocated rings?
> +
> + netif_device_attach(netdev);
> + rtnl_unlock();
> +}
^ permalink raw reply
* Re: [PATCH net-next v6 10/11] net: wangxun: improve flow control setting
From: Jakub Kicinski @ 2026-03-31 0:55 UTC (permalink / raw)
To: jiawenwu
Cc: Jakub Kicinski, netdev, mengyuanlou, andrew+netdev, davem,
edumazet, pabeni, richardcochran, linux, horms,
michal.swiatkowski, jacob.e.keller, kees, joe, larysa.zaremba,
abdun.nihaal, leitao
In-Reply-To: <20260326021406.30444-11-jiawenwu@trustnetic.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wangxun: improve flow control setting
Save the current mode of flow control, check the pause frame statistics
in watchdog to clear WX_HANG_CHECK_ARMED. The received pause frames are
divided into XON and XOFF to be counted, and Tx XON pause is disabled by
default for SP devices.
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index 2f71dc24306d4..4e0195f7df39e 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
[ ... ]
> @@ -2831,6 +2842,28 @@ int wx_fc_enable(struct wx *wx, bool tx_pause, bool rx_pause)
> }
> EXPORT_SYMBOL(wx_fc_enable);
>
> +static void wx_update_xoff_rx_lfc(struct wx *wx)
> +{
> + struct wx_hw_stats *hwstats = &wx->stats;
> + u64 data;
> + int i;
> +
> + if (wx->fc.mode != wx_fc_full &&
> + wx->fc.mode != wx_fc_rx_pause)
> + return;
> +
> + data = rd64(wx, WX_MAC_LXOFFRXC);
Is it possible that rd64() is reading a 32-bit register here?
Looking at wx_clear_hw_cntrs(), WX_MAC_LXOFFRXC is cleared using rd32().
If rd64() reads an adjacent unmapped address, could it populate the upper 32
bits of data with garbage?
> + hwstats->lxoffrxc += data;
> +
> + /* refill credits (no tx hang) if we received xoff */
> + if (!data)
> + return;
> +
> + for (i = 0; i < wx->num_tx_queues; i++)
> + clear_bit(WX_HANG_CHECK_ARMED,
> + wx->tx_ring[i]->state);
> +}
If data is populated with garbage from the upper 32 bits, would this bypass
the check for zero and unconditionally clear the WX_HANG_CHECK_ARMED bit on
all Tx queues, preventing the Tx hang recovery watchdog from operating?
> /**
> * wx_update_stats - Update the board statistics counters.
> * @wx: board private structure
> @@ -2844,6 +2877,10 @@ void wx_update_stats(struct wx *wx)
> u64 restart_queue = 0, tx_busy = 0;
> u32 i;
>
> + if (!netif_running(wx->netdev) ||
> + test_bit(WX_STATE_RESETTING, wx->state))
> + return;
> +
Does this early return cause the final hardware statistics to be lost on
interface teardown?
Since the explicit wx_update_stats() calls were removed from
ngbe_disable_device() and txgbe_disable_device(), and the interface is
marked down before hardware disablement completes, it seems any statistics
accumulated since the last periodic service task will be lost when the
registers are cleared on reset.
[ ... ]
> @@ -2893,7 +2932,10 @@ void wx_update_stats(struct wx *wx)
> hwstats->mptc += rd64(wx, WX_TX_MC_FRAMES_GOOD_L);
> hwstats->roc += rd32(wx, WX_RX_OVERSIZE_FRAMES_GOOD);
> hwstats->ruc += rd32(wx, WX_RX_UNDERSIZE_FRAMES_GOOD);
> - hwstats->lxonoffrxc += rd32(wx, WX_MAC_LXONOFFRXC);
> + if (wx->mac.type >= wx_mac_aml)
> + hwstats->lxonrxc = rd32(wx, WX_MAC_LXONRXC_AML);
Will this 64-bit software statistic wrap back to zero when the hardware
counter overflows?
As seen in wx_clear_hw_cntrs(), WX_MAC_LXONRXC_AML is cleared by writing 0:
wr32(wx, WX_MAC_LXONRXC_AML, 0);
This suggests it is a continuous 32-bit hardware counter rather than
clear-on-read. Direct assignment might break the monotonic property expected
by ethtool.
> + else
> + hwstats->lxonrxc += rd32(wx, WX_MAC_LXONRXC);
> hwstats->lxontxc += rd32(wx, WX_RDB_LXONTXC);
> hwstats->lxofftxc += rd32(wx, WX_RDB_LXOFFTXC);
> hwstats->o2bgptc += rd32(wx, WX_TDM_OS2BMC_CNT);
[ ... ]
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index b8db772c37c5c..080310a51ef86 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -150,6 +150,7 @@ static void ngbe_service_task(struct work_struct *work)
> struct wx *wx = container_of(work, struct wx, service_task);
>
> wx_handle_errors_subtask(wx);
> + wx_update_stats(wx);
>
> wx_service_event_complete(wx);
> }
Can this introduce data races with userspace ethtool callbacks?
The service task executes asynchronously without holding rtnl_lock, while
ethtool callbacks like wx_get_pause_stats() hold rtnl_lock() and also call
wx_update_stats().
This might lead to double-reads of clear-on-read hardware registers and
lost software counter increments.
Also, could this cause a concurrency issue during interface teardown?
During reconfiguration, ngbe_close() deletes the service timer but does not
call cancel_work_sync(&wx->service_task).
If the service task runs concurrently with wx_clear_interrupt_scheme(),
it might bypass early return checks and access wx->tx_ring[i] while it
is being freed and set to NULL, potentially causing a use-after-free or
NULL pointer dereference.
[ ... ]
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index d4f87fbb95655..e9439e4ebe61e 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -132,6 +132,7 @@ static void txgbe_service_task(struct work_struct *work)
> wx_handle_errors_subtask(wx);
> txgbe_module_detection_subtask(wx);
> txgbe_link_config_subtask(wx);
> + wx_update_stats(wx);
>
> wx_service_event_complete(wx);
> }
(Same questions apply to txgbe_service_task regarding data races and
teardown concurrency.)
[ ... ]
^ permalink raw reply
* Re: [PATCH net-next v6 09/11] net: wangxun: add Tx timeout process
From: Jakub Kicinski @ 2026-03-31 0:55 UTC (permalink / raw)
To: jiawenwu
Cc: Jakub Kicinski, netdev, mengyuanlou, andrew+netdev, davem,
edumazet, pabeni, richardcochran, linux, horms,
michal.swiatkowski, jacob.e.keller, kees, joe, larysa.zaremba,
abdun.nihaal, leitao
In-Reply-To: <20260326021406.30444-10-jiawenwu@trustnetic.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wangxun: add Tx timeout process
Implement .ndo_tx_timeout to handle Tx side timeout event. When Tx
timeout event occur, it will triger driver into reset process.
The WX_HANG_CHECK_ARMED bit is set to indicate a potential hang. It will
be cleared if a pause frame is received to remove false hang detection
due to 802.3 frames, which is implemented in later commit.
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_err.c b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> new file mode 100644
> index 0000000000000..bb77bdce69d22
> --- /dev/null
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_err.c
> @@ -0,0 +1,126 @@
[ ... ]
> +static void wx_reset_subtask(struct wx *wx)
> +{
> + if (!test_bit(WX_FLAG_NEED_PF_RESET, wx->flags))
> + return;
> +
> + if (!netif_running(wx->netdev) ||
> + test_bit(WX_STATE_RESETTING, wx->state))
> + return;
> +
> + rtnl_lock();
> +
> + wx_warn(wx, "Reset adapter.\n");
> +
> + if (test_bit(WX_FLAG_NEED_PF_RESET, wx->flags)) {
> + if (wx->do_reset)
> + wx->do_reset(wx->netdev, true);
Can this race with dev_close()?
If the interface is brought down concurrently, dev_close() acquires rtnl_lock(),
clears the running state, and frees resources. When wx_reset_subtask() later
acquires the lock, it does not re-check netif_running().
Could this result in calling do_reset() on a closed interface, leading to
hardware state inconsistencies or use-after-free crashes?
> + clear_bit(WX_FLAG_NEED_PF_RESET, wx->flags);
Could this unconditionally clear WX_FLAG_NEED_PF_RESET even if another
context sets it during the do_reset() execution? Should this use
test_and_clear_bit() before the reset operation to avoid losing concurrent
reset requests?
> + }
> +
> + rtnl_unlock();
> +}
[ ... ]
> +void wx_tx_timeout(struct net_device *netdev, unsigned int txqueue)
> +{
> + struct wx *wx = netdev_priv(netdev);
> + u32 head, tail;
> + int i;
> +
> + for (i = 0; i < wx->num_tx_queues; i++) {
> + struct wx_ring *tx_ring = wx->tx_ring[i];
> +
> + if (test_bit(WX_TX_DETECT_HANG, tx_ring->state) &&
> + wx_check_tx_hang(tx_ring))
Does calling wx_check_tx_hang() from the dev_watchdog softirq context cause
data races with the NAPI polling context (wx_clean_tx_irq)?
wx_check_tx_hang() modifies ring state bits (WX_HANG_CHECK_ARMED) and updates
the 64-bit statistic tx_done_old. Can this lead to torn reads or writes on
32-bit platforms and silent corruption of the hang detection state?
> + wx_warn(wx, "Real tx hang detected on queue %d\n", i);
> +
[ ... ]
> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index baffcadb22690..b8db772c37c5c 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
[ ... ]
> @@ -370,6 +391,7 @@ static void ngbe_disable_device(struct wx *wx)
> wx_napi_disable_all(wx);
> netif_tx_stop_all_queues(netdev);
> netif_tx_disable(netdev);
> + timer_delete_sync(&wx->service_timer);
Does this code leave the service task running?
timer_delete_sync() is called, but cancel_work_sync(&wx->service_task) is
missing. If the work is already queued, could wx_check_tx_hang_subtask() run
after the transmission and reception rings are freed, resulting in a
use-after-free when accessing wx->tx_ring[i]->state?
> if (wx->gpio_ctrl)
> ngbe_sfp_modules_txrx_powerctl(wx, false);
> wx_irq_disable(wx);
[ ... ]
> @@ -816,6 +845,9 @@ static void ngbe_remove(struct pci_dev *pdev)
> struct wx *wx = pci_get_drvdata(pdev);
> struct net_device *netdev;
>
> + timer_delete_sync(&wx->service_timer);
> + cancel_work_sync(&wx->service_task);
> +
> netdev = wx->netdev;
> wx_disable_sriov(wx);
> unregister_netdev(netdev);
Does this teardown order result in a use-after-free?
cancel_work_sync() is called before unregister_netdev(). Since the network
device is still active, could a concurrent timeout or interrupt re-queue the
service task before unregister_netdev() finishes? This might allow the
re-queued work item to execute and access the wx structure after it has been
freed by devres.
^ permalink raw reply
* Re: [PATCH net-next v6 08/11] net: wangxun: delete service_timer before cancel service_work
From: Jakub Kicinski @ 2026-03-31 0:55 UTC (permalink / raw)
To: jiawenwu
Cc: Jakub Kicinski, netdev, mengyuanlou, andrew+netdev, davem,
edumazet, pabeni, richardcochran, linux, horms,
michal.swiatkowski, jacob.e.keller, kees, joe, larysa.zaremba,
abdun.nihaal, leitao
In-Reply-To: <20260326021406.30444-9-jiawenwu@trustnetic.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wangxun: delete service_timer before cancel service_work
This commit reorders timer cancellation before work cancellation in the
device removal path to prevent new work from being queued after the
device goes down.
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> index ade2bfe563aaa..5565062c74713 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> @@ -48,6 +48,7 @@ void wxvf_remove(struct pci_dev *pdev)
> struct wx *wx = pci_get_drvdata(pdev);
> struct net_device *netdev;
>
> + timer_delete_sync(&wx->service_timer);
Is timer_delete_sync() the right API to use here? Since this is a device
teardown path, using timer_shutdown_sync() would permanently prevent the
timer from being re-armed by concurrent operations.
> cancel_work_sync(&wx->service_task);
> netdev = wx->netdev;
> unregister_netdev(netdev);
Does cancelling the timer and work before unregister_netdev() leave a race
window?
Since the network interface is still registered and hardware interrupts
are not yet disabled, could an interrupt or a concurrent interface up
operation call wx_service_event_schedule() and place wx->service_task
back into the workqueue?
If unregister_netdev() then completes without cancelling this newly
queued work, could the work execute after the wx structure is freed,
resulting in a use-after-free? Moving the sync cancellations after
unregister_netdev() might prevent this.
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index ddc7cea9d7c28..01ceaf270b706 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> @@ -936,6 +936,7 @@ static void txgbe_remove(struct pci_dev *pdev)
> struct txgbe *txgbe = wx->priv;
> struct net_device *netdev;
>
> + timer_delete_sync(&wx->service_timer);
> cancel_work_sync(&wx->service_task);
>
> netdev = wx->netdev;
The same questions regarding timer_shutdown_sync() and the potential race
window with unregister_netdev() apply to this removal function as well.
^ 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