* Re: [PATCH net 6/6] net/ncsi: validate GP payload lengths before parsing
From: Jakub Kicinski @ 2026-04-23 19:12 UTC (permalink / raw)
To: michael.bommarito
Cc: Jakub Kicinski, sam, fercerpav, netdev, davem, edumazet, pabeni,
horms, linux-kernel, stable
In-Reply-To: <20260422160342.1975093-7-michael.bommarito@gmail.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/ncsi: validate GP payload lengths before parsing
This commit bounds MAC and VLAN counts to software and GC-reported limits.
It adds validation to ensure the declared GP payload covers the fixed GP
prefix, consumed entries, and the checksum before parsing filter tables.
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 94354dca23eac..565d38fd4b92d 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
[ ... ]
> @@ -924,6 +926,14 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
> if (rsp->mac_cnt > mac_nbits || rsp->vlan_cnt > ncvf->n_vids)
> return -ERANGE;
Does this code read rsp->mac_cnt and rsp->vlan_cnt before validating the
payload length?
If a malformed packet with a payload length of 0 is received,
ncsi_validate_rsp_pkt() guarantees only 20 bytes (the common response
header) are pulled into the skb linear data area.
Since rsp->mac_cnt and rsp->vlan_cnt are at offsets 20 and 24 respectively,
could accessing them prior to the payload length check trigger an
out-of-bounds read?
> + payload = ncsi_rsp_payload(nr->rsp);
> + needed = offsetof(struct ncsi_rsp_gp_pkt, mac) - sizeof(rsp->rsp);
> + needed += mac_cnt * ETH_ALEN;
> + needed += vlan_cnt * sizeof(__be16);
> + needed += sizeof(rsp->checksum);
> + if (payload < needed)
> + return -EINVAL;
Are there missing timer cancellations during NCSI device unregistration
that could lead to a use-after-free regression?
ncsi_register_dev() initializes timers for 256 NCSI requests. If an NCSI
request is pending when the device is unregistered, and
ncsi_unregister_dev() calls kfree(ndp) without a prior timer_delete_sync(),
could the timer fire later and cause ncsi_request_timeout() to dereference
the freed memory?
Is there a synchronization issue between the workqueue teardown order and
device unregistration?
ncsi_unregister_dev() frees all packages and their channels via
ncsi_remove_package() before calling disable_work_sync(&ndp->work).
If the background task ncsi_dev_work() executes concurrently during this
teardown, could it attempt to suspend or configure the active channel
(e.g., in ncsi_suspend_channel()) and access ndp->active_package and
ndp->active_channel after they have already been freed?
Does the RCU synchronization in the package and channel removal paths properly
prevent use-after-free regressions during receive?
ncsi_remove_channel() and ncsi_remove_package() unlink elements from
RCU-protected lists (list_del_rcu(&nc->node)) and immediately call kfree()
without invoking synchronize_rcu() or using kfree_rcu().
Since the Rx path in ncsi_rcv_rsp() concurrently searches these lists using
list_for_each_entry_rcu(), could an incoming packet cause the Rx handler to
traverse and dereference recently freed package or channel memory?
Does ncsi_rsp_handler_gc() leak the MAC and VLAN filter arrays on repeated Get
Capabilities (GC) responses?
The handler unconditionally allocates memory for nc->mac_filter.addrs and
nc->vlan_filter.vids using kzalloc() and kcalloc(), but does not appear to
check for or free any previously allocated memory.
If a channel goes through a reset cycle via ncsi_reset_dev(), or if a GC
command is explicitly injected via Netlink, could the handler execute again
and overwrite the pointers, leaking the previous allocations?
^ permalink raw reply
* Re: [PATCH net 3/6] net/ncsi: validate GMCMA address counts against the payload
From: Jakub Kicinski @ 2026-04-23 19:12 UTC (permalink / raw)
To: michael.bommarito
Cc: Jakub Kicinski, sam, fercerpav, netdev, davem, edumazet, pabeni,
horms, linux-kernel, stable
In-Reply-To: <20260422160342.1975093-4-michael.bommarito@gmail.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/ncsi: validate GMCMA address counts against the payload
This commit adds payload length checks to the NC-SI GMCMA response handler
to ensure the advertised address count does not exceed the packet size.
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
[ ... ]
> @@ -1127,9 +1135,21 @@ static int ncsi_rsp_handler_gmcma(struct ncsi_request *nr)
> struct sockaddr_storage *saddr = &ndp->pending_mac;
> struct net_device *ndev = ndp->ndev.dev;
> struct ncsi_rsp_gmcma_pkt *rsp;
> + unsigned int addr_bytes;
> + unsigned int payload;
> int i;
>
> rsp = (struct ncsi_rsp_gmcma_pkt *)skb_network_header(nr->rsp);
> + payload = ncsi_rsp_payload(nr->rsp);
> + if (payload < sizeof(rsp->address_count) + sizeof(rsp->reserved) +
> + sizeof(__be32))
The commit message states it validates the fixed GMCMA fields plus checksum.
Does the NC-SI payload length actually include the 4-byte checksum?
Looking at ncsi_validate_rsp_pkt(), the common.length field does not include
the checksum, but it does include the 2-byte code and 2-byte reason fields.
Is it possible the subtraction of sizeof(__be32) here coincidentally works out
to the same 4 bytes as subtracting the code and reason fields, but conceptually
misrepresents the packet layout?
> + return -EINVAL;
> +
> + addr_bytes = payload - sizeof(rsp->address_count) -
> + sizeof(rsp->reserved) - sizeof(__be32);
> + if (rsp->address_count > addr_bytes / ETH_ALEN)
> + return -EINVAL;
> +
> ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
This isn't a bug introduced by this patch, but does this read-modify-write
of priv_flags in softirq context race with other modifications?
Since priv_flags is a standard unsigned int, could a concurrent update from
process context holding rtnl_lock (such as modifying IFF_PROMISC) silently
discard this IFF_LIVE_ADDR_CHANGE update or vice versa?
>
> netdev_info(ndev, "NCSI: Received %d provisioned MAC addresses\n",
This isn't a bug introduced by this patch, but are the updates to pending_mac
and gma_flag further down in this function safe from data races?
The caller ncsi_rcv_rsp() explicitly drops ndp->lock before invoking the
response handler:
ncsi_rcv_rsp()
spin_unlock_irqrestore(&ndp->lock, flags);
ret = nrh->handler(nr);
Since the NC-SI subsystem allows userspace to inject commands concurrently via
Netlink, could multiple Get MC MAC Address responses process simultaneously on
different CPUs and cause a torn write when copying the 6-byte MAC address into
ndp->pending_mac?
^ permalink raw reply
* Re: [PATCH net 2/6] net/ncsi: bound filter table state to software limits
From: Jakub Kicinski @ 2026-04-23 19:12 UTC (permalink / raw)
To: michael.bommarito
Cc: Jakub Kicinski, sam, fercerpav, netdev, davem, edumazet, pabeni,
horms, linux-kernel, stable
In-Reply-To: <20260422160342.1975093-3-michael.bommarito@gmail.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/ncsi: bound filter table state to software limits
The NCSI filter state uses single-word bitmaps for both MAC and VLAN
entries, but Get Capabilities and Get Parameters responses can still
feed larger counts into that state. This patch caps the stored table
sizes to the bitmap width to prevent out-of-bounds accesses.
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 1fe061ede26dd..47ddf2bbb13bb 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
[ ... ]
> @@ -819,6 +829,12 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
> nc->caps[NCSI_CAP_VLAN].cap = rsp->vlan_mode &
> NCSI_CAP_VLAN_MASK;
>
> + vlan_cnt = min_t(unsigned int, rsp->vlan_cnt, NCSI_FILTER_BITS);
> + if (vlan_cnt != rsp->vlan_cnt)
> + netdev_warn(ndp->ndev.dev,
> + "NCSI: VLAN filter count %u exceeds software limit %u\n",
> + rsp->vlan_cnt, (unsigned int)NCSI_FILTER_BITS);
> +
> size = (rsp->uc_cnt + rsp->mc_cnt + rsp->mixed_cnt) * ETH_ALEN;
> nc->mac_filter.addrs = kzalloc(size, GFP_ATOMIC);
> if (!nc->mac_filter.addrs)
> @@ -827,7 +843,7 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
> nc->mac_filter.n_mc = rsp->mc_cnt;
> nc->mac_filter.n_mixed = rsp->mixed_cnt;
>
> - nc->vlan_filter.vids = kcalloc(rsp->vlan_cnt,
> + nc->vlan_filter.vids = kcalloc(vlan_cnt,
> sizeof(*nc->vlan_filter.vids),
> GFP_ATOMIC);
> if (!nc->vlan_filter.vids)
> @@ -836,7 +852,7 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
> * configuration state
> */
> nc->vlan_filter.bitmap = U64_MAX;
> - nc->vlan_filter.n_vids = rsp->vlan_cnt;
> + nc->vlan_filter.n_vids = vlan_cnt;
[ ... ]
> @@ -862,6 +881,15 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
> if (!nc)
> return -ENODEV;
>
> + ncmf = &nc->mac_filter;
> + ncvf = &nc->vlan_filter;
> + mac_cnt = min_t(unsigned int, rsp->mac_cnt, NCSI_FILTER_BITS);
> + mac_nbits = ncmf->n_uc + ncmf->n_mc + ncmf->n_mixed;
> + vlan_cnt = min_t(unsigned int, rsp->vlan_cnt, ncvf->n_vids);
> +
> + if (rsp->mac_cnt > mac_nbits || rsp->vlan_cnt > ncvf->n_vids)
> + return -ERANGE;
Because nc->vlan_filter.n_vids is capped to 64 in the GC response handler,
could this check entirely reject GP responses from compliant hardware that
supports and reports more than 64 VLAN filters?
> /* Modes with explicit enabled indications */
> if (ntohl(rsp->valid_modes) & 0x1) { /* BC filter mode */
[ ... ]
> @@ -887,11 +915,11 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
> /* MAC addresses filter table */
> pdata = (unsigned char *)rsp + 48;
> enable = rsp->mac_enable;
> - ncmf = &nc->mac_filter;
> spin_lock_irqsave(&nc->lock, flags);
> bitmap = &ncmf->bitmap;
> - for (i = 0; i < rsp->mac_cnt; i++, pdata += 6) {
> - if (!(enable & (0x1 << i)))
> + for (i = 0; i < mac_cnt; i++, pdata += 6) {
> + if (!ncsi_filter_is_enabled(enable, i,
> + BITS_PER_TYPE(rsp->mac_enable)))
> clear_bit(i, bitmap);
> else
> set_bit(i, bitmap);
If the device reports a MAC count greater than 64 but still within the
software limit of mac_nbits, this loop will terminate at 64 due to the
mac_cnt cap.
Will this cause pdata to be advanced by only 384 bytes instead of the
actual size of the MAC array in the packet?
> @@ -902,11 +930,11 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
>
> /* VLAN filter table */
> enable = ntohs(rsp->vlan_enable);
> - ncvf = &nc->vlan_filter;
> bitmap = &ncvf->bitmap;
> spin_lock_irqsave(&nc->lock, flags);
> - for (i = 0; i < rsp->vlan_cnt; i++, pdata += 2) {
> - if (!(enable & (0x1 << i)))
> + for (i = 0; i < vlan_cnt; i++, pdata += 2) {
> + if (!ncsi_filter_is_enabled(enable, i,
> + BITS_PER_TYPE(rsp->vlan_enable)))
If pdata is not advanced past all MAC addresses from the payload, does this
cause the VLAN filter loop to misinterpret the remaining MAC address bytes
as VLAN tags?
While reviewing this patch, I noticed a few pre-existing regressions in the
surrounding codebase that might be worth looking into.
In ncsi_unregister_dev(), the subsystem frees all packages and channels by
iterating and calling ncsi_remove_package(), and then calls
disable_work_sync(&ndp->work). If ncsi_dev_work is executing concurrently,
could it dereference ndp->active_package and ndp->active_channel after they
have been freed?
Also in ncsi_unregister_dev(), ndp is freed using kfree(), but it doesn't
appear to iterate over ndp->requests to cancel their associated timers via
del_timer_sync(). If a pending NCSI request times out after the device is
unregistered, could the timer callback dereference the freed ndp pointer?
Finally, ncsi_remove_package() and ncsi_remove_channel() unlink objects
using list_del_rcu(), but seem to immediately free the objects using kfree()
instead of kfree_rcu() or synchronize_rcu(). Could concurrent readers
traversing these lists under rcu_read_lock() access freed memory?
^ permalink raw reply
* Re: [PATCH net 1/6] net/ncsi: validate response packet lengths against the skb
From: Jakub Kicinski @ 2026-04-23 19:12 UTC (permalink / raw)
To: michael.bommarito
Cc: Jakub Kicinski, sam, fercerpav, netdev, davem, edumazet, pabeni,
horms, linux-kernel, stable
In-Reply-To: <20260422160342.1975093-2-michael.bommarito@gmail.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/ncsi: validate response packet lengths against the skb
This commit adds packet length validation for NC-SI responses. It pulls the
common header first, then pulls the full response body before validating the
packet to prevent reading past the received packet body.
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index fbd84bc8026a3..1fe061ede26dd 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -38,11 +38,18 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
> struct ncsi_rsp_pkt_hdr *h;
> u32 checksum;
> __be32 *pchecksum;
> + unsigned int len;
>
> /* Check NCSI packet header. We don't need validate
> * the packet type, which should have been checked
> * before calling this function.
> */
> + len = skb_network_offset(nr->rsp) + sizeof(*h) + ALIGN(payload, 4);
> + if (!pskb_may_pull(nr->rsp, len)) {
Does this pskb_may_pull() call potentially invalidate the hdr pointer cached
earlier in ncsi_rcv_rsp()?
Looking at ncsi_rcv_rsp(), it caches the header pointer before validating:
ncsi_rcv_rsp() {
...
hdr = (struct ncsi_pkt_hdr *)skb_network_header(skb);
...
ret = ncsi_validate_rsp_pkt(nr, payload);
If the SKB is fragmented and pskb_may_pull() needs to pull data into the
linear area, it can reallocate the SKB's data buffer and free the old buffer.
When ncsi_validate_rsp_pkt() returns, the local hdr pointer in ncsi_rcv_rsp()
would still point to the freed memory.
This pointer is then dereferenced in multiple error paths:
if (ret) {
netdev_warn(ndp->ndev.dev,
"NCSI: 'bad' packet ignored for type 0x%x\n",
hdr->type);
...
ret = nrh->handler(nr);
if (ret)
netdev_err(ndp->ndev.dev,
"NCSI: Handler for packet type 0x%x returned %d\n",
hdr->type, ret);
Could this cause a use-after-free when reading hdr->type?
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH net v3] ipv6: Cap TLV scan in ip6_tnl_parse_tlv_enc_lim
From: patchwork-bot+netdevbpf @ 2026-04-23 19:10 UTC (permalink / raw)
To: Daniel Borkmann
Cc: kuba, edumazet, dsahern, tom, willemdebruijn.kernel, idosch,
justin.iurman, pabeni, netdev
In-Reply-To: <20260421202406.717885-1-daniel@iogearbox.net>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Tue, 21 Apr 2026 22:24:06 +0200 you wrote:
> Commit 47d3d7ac656a ("ipv6: Implement limits on Hop-by-Hop and
> Destination options") added net.ipv6.max_{hbh,dst}_opts_{cnt,len}
> and applied them in ip6_parse_tlv(), the generic TLV walker
> invoked from ipv6_destopt_rcv() and ipv6_parse_hopopts().
>
> ip6_tnl_parse_tlv_enc_lim() does not go through ip6_parse_tlv();
> it has its own hand-rolled TLV scanner inside its NEXTHDR_DEST
> branch which looks for IPV6_TLV_TNL_ENCAP_LIMIT. That inner
> loop is bounded only by optlen, which can be up to 2048 bytes.
> Stuffing the Destination Options header with 2046 Pad1 (type=0)
> entries advances the scanner a single byte at a time, yielding
> ~2000 TLV iterations per extension header.
>
> [...]
Here is the summary with links:
- [net,v3] ipv6: Cap TLV scan in ip6_tnl_parse_tlv_enc_lim
https://git.kernel.org/netdev/net/c/076b8cad77aa
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 v4 0/2] tcp: fix listener wakeup after reuseport migration
From: patchwork-bot+netdevbpf @ 2026-04-23 19:10 UTC (permalink / raw)
To: Zhenzhong Wu
Cc: netdev, edumazet, ncardwell, kuniyu, davem, dsahern, kuba, pabeni,
horms, shuah, tamird, linux-kernel, linux-kselftest
In-Reply-To: <20260422024554.130346-1-jt26wzz@gmail.com>
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 22 Apr 2026 10:45:52 +0800 you wrote:
> This series fixes a missing wakeup when inet_csk_listen_stop() migrates
> an established child socket from a closing listener to another socket
> in the same SO_REUSEPORT group after the child has already been queued
> for accept.
>
> The target listener receives the migrated accept-queue entry via
> inet_csk_reqsk_queue_add(), but its waiters are not notified.
> Nonblocking accept() still succeeds because it checks the accept queue
> directly, but readiness-based waiters can remain asleep until another
> connection generates a wakeup.
>
> [...]
Here is the summary with links:
- [net,v4,1/2] tcp: call sk_data_ready() after listener migration
https://git.kernel.org/netdev/net/c/3864c6ba1e04
- [net,v4,2/2] selftests/bpf: check epoll readiness during reuseport migration
https://git.kernel.org/netdev/net/c/c01cfc488675
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 v1] vhost_net: fix sleeping with preempt-disabled in vhost_net_busy_poll()
From: patchwork-bot+netdevbpf @ 2026-04-23 19:10 UTC (permalink / raw)
To: Kohei Enju
Cc: mst, jasowang, eperezma, kvm, virtualization, netdev,
syzbot+6985cb8e543ea90ba8ee
In-Reply-To: <20260422023026.81960-1-kohei@enjuk.jp>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 22 Apr 2026 02:30:24 +0000 you wrote:
> syzbot reported "sleeping function called from invalid context" in
> vhost_net_busy_poll().
>
> Commit 030881372460 ("vhost_net: basic polling support") introduced a
> busy-poll loop and preempt_{disable,enable}() around it, where each
> iteration calls a sleepable function inside the loop.
>
> [...]
Here is the summary with links:
- [net,v1] vhost_net: fix sleeping with preempt-disabled in vhost_net_busy_poll()
https://git.kernel.org/netdev/net/c/e08a9fac5cf8
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] m68k: mvme147: Make me the maintainer
From: patchwork-bot+netdevbpf @ 2026-04-23 19:10 UTC (permalink / raw)
To: Daniel Palmer
Cc: andrew+netdev, geert, James.Bottomley, martin.petersen, davem,
edumazet, kuba, pabeni, linux-m68k, linux-scsi, netdev,
linux-kernel
In-Reply-To: <20260422132710.2855826-1-daniel@thingy.jp>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 22 Apr 2026 22:27:10 +0900 you wrote:
> I'm actively using mainline + patches on this board as a bootloader
> for another VME board and as a terminal server using a multiport
> serial board in the same VME backplane. I even have mainline u-boot
> on real EPROMs.
>
> Make me the maintainer of its ethernet, scsi and arch code so I get
> an email before one or more of them get deleted.
>
> [...]
Here is the summary with links:
- m68k: mvme147: Make me the maintainer
https://git.kernel.org/netdev/net/c/7256eb3e0909
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] net: txgbe: fix firmware version check
From: patchwork-bot+netdevbpf @ 2026-04-23 19:10 UTC (permalink / raw)
To: Jiawen Wu
Cc: netdev, mengyuanlou, andrew+netdev, davem, edumazet, kuba, pabeni,
horms, jacob.e.keller, stable
In-Reply-To: <C787AA5C07598B13+20260422071837.372731-1-jiawenwu@trustnetic.com>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 22 Apr 2026 15:18:37 +0800 you wrote:
> For the device SP, the firmware version is a 32-bit value where the
> lower 20 bits represent the base version number. And the customized
> firmware version populates the upper 12 bits with a specific
> identification number.
>
> For other devices AML 25G and 40G, the upper 12 bits of the firmware
> version is always non-zero, and they have other naming conventions.
>
> [...]
Here is the summary with links:
- [net] net: txgbe: fix firmware version check
https://git.kernel.org/netdev/net/c/c263f644add3
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 v2 1/1] tipc: fix double-free in tipc_buf_append()
From: patchwork-bot+netdevbpf @ 2026-04-23 19:10 UTC (permalink / raw)
To: Lee Jones
Cc: jmaloy, davem, edumazet, kuba, pabeni, horms, ying.xue, netdev,
tipc-discussion, linux-kernel, tung.quang.nguyen
In-Reply-To: <20260421124528.162996-1-lee@kernel.org>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Tue, 21 Apr 2026 13:45:26 +0100 you wrote:
> tipc_msg_validate() can potentially reallocate the skb it is validating,
> freeing the old one. In tipc_buf_append(), it was being called with a
> pointer to a local variable which was a copy of the caller's skb
> pointer.
>
> If the skb was reallocated and validation subsequently failed, the error
> handling path would free the original skb pointer, which had already
> been freed, leading to double-free.
>
> [...]
Here is the summary with links:
- [v2,1/1] tipc: fix double-free in tipc_buf_append()
https://git.kernel.org/netdev/net/c/d293ca716e7d
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 v2 1/1] tipc: fix double-free in tipc_buf_append()
From: Simon Horman @ 2026-04-23 19:10 UTC (permalink / raw)
To: Lee Jones
Cc: Jon Maloy, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Ying Xue, netdev, tipc-discussion, linux-kernel,
Tung Nguyen
In-Reply-To: <20260421124528.162996-1-lee@kernel.org>
On Tue, Apr 21, 2026 at 01:45:26PM +0100, Lee Jones wrote:
> tipc_msg_validate() can potentially reallocate the skb it is validating,
> freeing the old one. In tipc_buf_append(), it was being called with a
> pointer to a local variable which was a copy of the caller's skb
> pointer.
>
> If the skb was reallocated and validation subsequently failed, the error
> handling path would free the original skb pointer, which had already
> been freed, leading to double-free.
>
> Fix this by checking if head now points to a newly allocated reassembled
> skb. If it does, reassign *headbuf for later freeing operations.
>
> Fixes: d618d09a68e4 ("tipc: enforce valid ratio between skb truesize and contents")
> Suggested-by: Tung Nguyen <tung.quang.nguyen@est.tech>
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
> 1v => v2: Keep the passed pointer type the same, but reassign on-change
FTR: Sashiko has generated a review of this patch which I have examined.
I do not believe that review should halt progress of this patch
as it appears that the problem flagged pre-dates this patch. Actually,
its unclear to me if it is a problem that warrants addressing at all.
But I'd appreciate if it could be looked over as a follow-up task.
^ permalink raw reply
* Re: [PATCH v1 net] udp: Use READ_ONCE()/WRITE_ONCE() for hslot->count.
From: Eric Dumazet @ 2026-04-23 18:59 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Willem de Bruijn,
Simon Horman, David Held, Kuniyuki Iwashima, netdev,
syzbot+04905b8b3523856476c0
In-Reply-To: <CAAVpQUDt+Ox3Q0Lu7SVH5doFrTxFJOSigfP5E1WGB0Bi102S4Q@mail.gmail.com>
On Thu, Apr 23, 2026 at 11:51 AM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>
> On Thu, Apr 23, 2026 at 12:29 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Wed, Apr 22, 2026 at 6:09 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > >
> > > __udp4_lib_mcast_demux_lookup() and __udp4_lib_mcast_deliver()
> > > reads the number of sockets in the 1-tuple hash table chain
> > > locklessly. [0]
> > >
> > > Let's use READ_ONCE() and WRITE_ONCE() for hslot->count.
> > >
> > > [0]:
> > > BUG: KCSAN: data-race in __udp4_lib_mcast_deliver / udp_lib_unhash
> >
> > ...
> >
> > >
> > > Reported by Kernel Concurrency Sanitizer on:
> > > CPU: 1 UID: 0 PID: 15111 Comm: syz.6.4060 Not tainted syzkaller #0 PREEMPT(full)
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2026
> > >
> > > Fixes: 2dc41cff7545 ("udp: Use hash2 for long hash1 chains in __udp*_lib_mcast_deliver.")
> > > Fixes: 63c6f81cdde5 ("udp: ipv4: do not waste time in __udp4_lib_mcast_demux_lookup")
> > > Reported-by: syzbot+04905b8b3523856476c0@syzkaller.appspotmail.com
> > > Closes: https://lore.kernel.org/netdev/69e97093.050a0220.24bfd3.0048.GAE@google.com/
> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
> > > ---
> >
> > You forgot to change net/ipv6/udp.c
>
> Ah exactly. Will add this in v2.
>
> >
> > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> > index 15e032194eccc3c29b3e5f3a09214cad60623329..1f4dabe4c350118441f61c1cf0398e698581542c
> > 100644
> > --- a/net/ipv6/udp.c
> > +++ b/net/ipv6/udp.c
> > @@ -953,7 +953,7 @@ static int __udp6_lib_mcast_deliver(struct net
> > *net, struct sk_buff *skb,
> > hash2_any = 0;
> > hash2 = 0;
> > hslot = udp_hashslot(udptable, net, hnum);
> > - use_hash2 = hslot->count > 10;
> > + use_hash2 = READ_ONCE(hslot->count) > 10;
> > offset = offsetof(typeof(*sk), sk_node);
> >
> > if (use_hash2) {
> >
> >
> > Also one part is missing in udp_lib_get_port()
>
> I excluded this part since it's under spin_lock_bh(&hslot->lock).
>
But you would need a spin_lock(hslot2->lock), wich we do not hold.
> >
> > @@ -289,7 +289,7 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum,
> > hash2_nulladdr &= udptable->mask;
> >
> > hslot2 = udp_hashslot2(udptable, slot2);
> > - if (hslot->count < hslot2->count)
> > + if (hslot->count < READ_ONCE(hslot2->count))
> > goto scan_primary_hash;
> >
> > exist = udp_lib_lport_inuse2(net, snum, hslot2, sk);
^ permalink raw reply
* Re: [PATCH net v2 0/6] rxrpc: Miscellaneous fixes
From: Jakub Kicinski @ 2026-04-23 18:58 UTC (permalink / raw)
To: David Howells
Cc: netdev, Marc Dionne, David S. Miller, Eric Dumazet, Paolo Abeni,
Simon Horman, Anderson Nascimento, linux-afs, linux-kernel
In-Reply-To: <2990993.1776970042@warthog.procyon.org.uk>
On Thu, 23 Apr 2026 19:47:22 +0100 David Howells wrote:
> Jakub Kicinski <kuba@kernel.org> wrote:
>
> > Have you had a chance to look thru sashiko run for this?
> >
> > https://sashiko.dev/#/patchset/20260422161438.2593376-4-dhowells@redhat.com
>
> It wasn't available this morning when I looked. I know there are a couple of
> things to fix from other checks.
It is now :) May I offer the following menu of options
- all the complaints are already taken into account / intentional /
will be addressed later
- I need more time, let's wait until tomorrow
- some of it looks legit, apply patches ${list} only
- let me respin completely
^ permalink raw reply
* Re: [PATCH net] iavf: iavf_virtchnl_completion: drop duplicate ether_addr_equal() test
From: Simon Horman @ 2026-04-23 18:55 UTC (permalink / raw)
To: Corinna Vinschen
Cc: intel-wired-lan, netdev, Jose Ignacio Tornos Martinez,
Michal Schmidt, stable
In-Reply-To: <20260421111236.875379-1-vinschen@redhat.com>
On Tue, Apr 21, 2026 at 01:12:36PM +0200, Corinna Vinschen wrote:
> This is just a simple cleanup fix. Commit 35a2443d0910f ("iavf: Add
> waiting for response from PF in set mac") introduced a duplicate
> ether_addr_equal() check, so the current code tests the new MAC twice
> against the former MAC.
>
> Remove the outer ether_addr_equal() test, remnant of commit c5c922b3e09b
> ("iavf: fix MAC address setting for VFs when filter is rejected")
>
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> Fixes: 35a2443d0910f ("iavf: Add waiting for response from PF in set mac")
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
> Added CC: stable@vger.kernel.org
Hi,
This feels more like a cleanup for net-next (without a Fixes tag)
than a fix for net. I'm missing where the bug is here.
^ permalink raw reply
* Re: [PATCH net v2 1/1] net/sched: cls_flower: avoid stale mask references after delete
From: Jakub Kicinski @ 2026-04-23 18:51 UTC (permalink / raw)
To: jiri
Cc: Ren Wei, netdev, jhs, davem, edumazet, pabeni, horms, sbrivio,
vladbu, yuantan098, yifanwucs, tomapufckgml, bird, kanolyc,
z1652074432
In-Reply-To: <20260421160303.1919562-1-n05ec@lzu.edu.cn>
On Wed, 22 Apr 2026 00:03:02 +0800 Ren Wei wrote:
> From: Yuhang Zheng <z1652074432@gmail.com>
>
> cls_flower keeps filter and mask state separately. After a filter is
> removed or replaced, some paths can still need the mask data associated
> with that filter.
>
> Cache the mask key and dissector in struct cls_fl_filter when the mask
> is assigned, and use the cached copies in dump and offload paths. This
> avoids depending on the external mask object's lifetime after delete or
> replace.
>
> Fixes: 92149190067d ("net: sched: flower: set unlocked flag for flower proto ops")
> Cc: stable@kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Tested-by: Yucheng Lu <kanolyc@gmail.com>
> Signed-off-by: Yuhang Zheng <z1652074432@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
Jiri, do you have an opinion? Feels slightly wasteful (as sashiko points
out), IDK if there's a cleaner fix.
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 099ff6a3e1f5..c1f10b4ec748 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -124,8 +124,10 @@ struct cls_fl_head {
>
> struct cls_fl_filter {
> struct fl_flow_mask *mask;
> + struct flow_dissector mask_dissector;
> struct rhash_head ht_node;
> struct fl_flow_key mkey;
> + struct fl_flow_key mask_key;
> struct tcf_exts exts;
> struct tcf_result res;
> struct fl_flow_key key;
> @@ -445,6 +447,12 @@ static void fl_destroy_filter_work(struct work_struct *work)
> __fl_destroy_filter(f);
> }
>
> +static void fl_filter_copy_mask(struct cls_fl_filter *f)
> +{
> + f->mask_key = f->mask->key;
> + f->mask_dissector = f->mask->dissector;
> +}
> +
> static void fl_hw_destroy_filter(struct tcf_proto *tp, struct cls_fl_filter *f,
> bool rtnl_held, struct netlink_ext_ack *extack)
> {
> @@ -476,8 +484,8 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
> tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, extack);
> cls_flower.command = FLOW_CLS_REPLACE;
> cls_flower.cookie = (unsigned long) f;
> - cls_flower.rule->match.dissector = &f->mask->dissector;
> - cls_flower.rule->match.mask = &f->mask->key;
> + cls_flower.rule->match.dissector = &f->mask_dissector;
> + cls_flower.rule->match.mask = &f->mask_key;
> cls_flower.rule->match.key = &f->mkey;
> cls_flower.classid = f->res.classid;
>
> @@ -2489,6 +2497,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
> err = fl_check_assign_mask(head, fnew, fold, mask);
> if (err)
> goto unbind_filter;
> + fl_filter_copy_mask(fnew);
>
> err = fl_ht_insert_unique(fnew, fold, &in_ht);
> if (err)
> @@ -2705,8 +2714,8 @@ static int fl_reoffload(struct tcf_proto *tp, bool add, flow_setup_cb_t *cb,
> cls_flower.command = add ?
> FLOW_CLS_REPLACE : FLOW_CLS_DESTROY;
> cls_flower.cookie = (unsigned long)f;
> - cls_flower.rule->match.dissector = &f->mask->dissector;
> - cls_flower.rule->match.mask = &f->mask->key;
> + cls_flower.rule->match.dissector = &f->mask_dissector;
> + cls_flower.rule->match.mask = &f->mask_key;
> cls_flower.rule->match.key = &f->mkey;
>
> err = tc_setup_offload_action(&cls_flower.rule->action, &f->exts,
> @@ -3709,7 +3718,7 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, void *fh,
> goto nla_put_failure_locked;
>
> key = &f->key;
> - mask = &f->mask->key;
> + mask = &f->mask_key;
> skip_hw = tc_skip_hw(f->flags);
>
> if (fl_dump_key(skb, net, key, mask))
^ permalink raw reply
* Re: [PATCH v1 net] udp: Use READ_ONCE()/WRITE_ONCE() for hslot->count.
From: Kuniyuki Iwashima @ 2026-04-23 18:51 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Willem de Bruijn,
Simon Horman, David Held, Kuniyuki Iwashima, netdev,
syzbot+04905b8b3523856476c0
In-Reply-To: <CANn89iKwALD=b-FqwQe=GjB23u5ksdepOZG+3PCjMESBqSw1bg@mail.gmail.com>
On Thu, Apr 23, 2026 at 12:29 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Apr 22, 2026 at 6:09 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> >
> > __udp4_lib_mcast_demux_lookup() and __udp4_lib_mcast_deliver()
> > reads the number of sockets in the 1-tuple hash table chain
> > locklessly. [0]
> >
> > Let's use READ_ONCE() and WRITE_ONCE() for hslot->count.
> >
> > [0]:
> > BUG: KCSAN: data-race in __udp4_lib_mcast_deliver / udp_lib_unhash
>
> ...
>
> >
> > Reported by Kernel Concurrency Sanitizer on:
> > CPU: 1 UID: 0 PID: 15111 Comm: syz.6.4060 Not tainted syzkaller #0 PREEMPT(full)
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2026
> >
> > Fixes: 2dc41cff7545 ("udp: Use hash2 for long hash1 chains in __udp*_lib_mcast_deliver.")
> > Fixes: 63c6f81cdde5 ("udp: ipv4: do not waste time in __udp4_lib_mcast_demux_lookup")
> > Reported-by: syzbot+04905b8b3523856476c0@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/netdev/69e97093.050a0220.24bfd3.0048.GAE@google.com/
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
> > ---
>
> You forgot to change net/ipv6/udp.c
Ah exactly. Will add this in v2.
>
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 15e032194eccc3c29b3e5f3a09214cad60623329..1f4dabe4c350118441f61c1cf0398e698581542c
> 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -953,7 +953,7 @@ static int __udp6_lib_mcast_deliver(struct net
> *net, struct sk_buff *skb,
> hash2_any = 0;
> hash2 = 0;
> hslot = udp_hashslot(udptable, net, hnum);
> - use_hash2 = hslot->count > 10;
> + use_hash2 = READ_ONCE(hslot->count) > 10;
> offset = offsetof(typeof(*sk), sk_node);
>
> if (use_hash2) {
>
>
> Also one part is missing in udp_lib_get_port()
I excluded this part since it's under spin_lock_bh(&hslot->lock).
>
> @@ -289,7 +289,7 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum,
> hash2_nulladdr &= udptable->mask;
>
> hslot2 = udp_hashslot2(udptable, slot2);
> - if (hslot->count < hslot2->count)
> + if (hslot->count < READ_ONCE(hslot2->count))
> goto scan_primary_hash;
>
> exist = udp_lib_lport_inuse2(net, snum, hslot2, sk);
^ permalink raw reply
* Re: [PATCH net v2] ipv4: icmp: validate reply type before using icmp_pointers
From: patchwork-bot+netdevbpf @ 2026-04-23 18:50 UTC (permalink / raw)
To: Ren Wei
Cc: netdev, edumazet, davem, dsahern, kuba, pabeni, horms,
andreas.a.roeseler, yuantan098, yifanwucs, tomapufckgml, bird,
caoruide123
In-Reply-To: <0dace90c01a5978e829ca741ef684dbd7304ce62.1776628519.git.caoruide123@gmail.com>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Tue, 21 Apr 2026 12:16:31 +0800 you wrote:
> From: Ruide Cao <caoruide123@gmail.com>
>
> Extended echo replies use ICMP_EXT_ECHOREPLY as the outbound reply type.
> That value is outside the range covered by icmp_pointers[], which only
> describes the traditional ICMP types up to NR_ICMP_TYPES.
>
> Avoid consulting icmp_pointers[] for reply types outside that range, and
> use array_index_nospec() for the remaining in-range lookup. Normal ICMP
> replies keep their existing behavior unchanged.
>
> [...]
Here is the summary with links:
- [net,v2] ipv4: icmp: validate reply type before using icmp_pointers
https://git.kernel.org/netdev/net/c/67bf002a2d73
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 v3] llc: Return -EINPROGRESS from llc_ui_connect()
From: patchwork-bot+netdevbpf @ 2026-04-23 18:50 UTC (permalink / raw)
To: Ernestas Kulik; +Cc: netdev, kuba, linux-kernel
In-Reply-To: <20260421060304.285419-1-ernestas.k@iconn-networks.com>
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Tue, 21 Apr 2026 09:02:26 +0300 you wrote:
> Given a zero sk_sndtimeo, llc_ui_connect() skips waiting for state
> change and returns 0, confusing userspace applications that will assume
> the socket is connected, making e.g. getpeername() calls error out.
>
> More specifically, the issue was discovered in libcoap, where
> newly-added AF_LLC socket support was behaving differently from AF_INET
> connections due to EINPROGRESS handling being skipped.
>
> [...]
Here is the summary with links:
- [v3] llc: Return -EINPROGRESS from llc_ui_connect()
https://git.kernel.org/netdev/net/c/864ba40c80ed
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 v3 0/2] tcp: symmetric challenge ACK for SEG.ACK > SND.NXT
From: patchwork-bot+netdevbpf @ 2026-04-23 18:50 UTC (permalink / raw)
To: Jiayuan Chen
Cc: netdev, edumazet, ncardwell, kuniyu, davem, dsahern, kuba, pabeni,
horms, shuah, linux-kernel, linux-kselftest
In-Reply-To: <20260422123605.320000-1-jiayuan.chen@linux.dev>
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 22 Apr 2026 20:35:37 +0800 you wrote:
> Commit 354e4aa391ed ("tcp: RFC 5961 5.2 Blind Data Injection Attack
> Mitigation") quotes RFC 5961 Section 5.2 in full, which requires
> that any incoming segment whose ACK value falls outside
> [SND.UNA - MAX.SND.WND, SND.NXT] MUST be discarded and an ACK sent
> back. Linux currently sends that challenge ACK only on the lower
> edge (SEG.ACK < SND.UNA - MAX.SND.WND); on the symmetric upper edge
> (SEG.ACK > SND.NXT) the segment is silently dropped with
> SKB_DROP_REASON_TCP_ACK_UNSENT_DATA.
>
> [...]
Here is the summary with links:
- [net,v3,1/2] tcp: send a challenge ACK on SEG.ACK > SND.NXT
https://git.kernel.org/netdev/net/c/42726ec644cb
- [net,v3,2/2] selftests/net: packetdrill: cover RFC 5961 5.2 challenge ACK on both edges
https://git.kernel.org/netdev/net/c/cf94b3c0f052
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 v2 0/6] rxrpc: Miscellaneous fixes
From: David Howells @ 2026-04-23 18:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: dhowells, netdev, Marc Dionne, David S. Miller, Eric Dumazet,
Paolo Abeni, Simon Horman, Anderson Nascimento, linux-afs,
linux-kernel
In-Reply-To: <20260423105639.6e0a0a7e@kernel.org>
Jakub Kicinski <kuba@kernel.org> wrote:
> Have you had a chance to look thru sashiko run for this?
>
> https://sashiko.dev/#/patchset/20260422161438.2593376-4-dhowells@redhat.com
It wasn't available this morning when I looked. I know there are a couple of
things to fix from other checks.
David
^ permalink raw reply
* Re: [PATCH] net: usb: rtl8150: free skb on usb_submit_urb() failure in xmit
From: Jakub Kicinski @ 2026-04-23 18:44 UTC (permalink / raw)
To: Morduan Zang
Cc: Petko Manolov, Andrew Lunn, David S . Miller, Eric Dumazet,
Paolo Abeni, linux-usb, netdev, linux-kernel
In-Reply-To: <678BC10BB9E39322+20260421111025.15833-1-zhangdandan@uniontech.com>
On Tue, 21 Apr 2026 19:10:25 +0800 Morduan Zang wrote:
> When rtl8150_start_xmit() fails to submit the tx URB, the URB is never
> handed to the USB core and write_bulk_callback() will not run. The
> driver returns NETDEV_TX_OK, which tells the networking stack that the
> skb has been consumed, but nothing actually frees the skb on this
> error path:
Does not apply, please rebase on net/main and repost.
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH bpf-next v3 4/9] bpf: Refactor object relationship tracking and fix dynptr UAF bug
From: Amery Hung @ 2026-04-23 18:44 UTC (permalink / raw)
To: Mykyta Yatsenko
Cc: bpf, netdev, alexei.starovoitov, andrii, daniel, eddyz87, memxor,
martin.lau, kernel-team
In-Reply-To: <4de82d94-a975-4703-b45e-8932dfc28248@gmail.com>
On Thu, Apr 23, 2026 at 11:19 AM Mykyta Yatsenko
<mykyta.yatsenko5@gmail.com> wrote:
>
>
>
> On 4/21/26 11:10 PM, Amery Hung wrote:
> > Refactor object relationship tracking in the verifier and fix a dynptr
> > use-after-free bug where file/skb dynptrs are not invalidated when the
> > parent referenced object is freed.
> >
> > Add parent_id to bpf_reg_state to precisely track child-parent
> > relationships. A child object's parent_id points to the parent object's
> > id. This replaces the PTR_TO_MEM-specific dynptr_id and does not
> > increase the size of bpf_reg_state on 64-bit machines as there is
> > existing padding.
> >
> > When calling dynptr constructors (i.e., process_dynptr_func() with
> > MEM_UNINIT argument), track the parent's id if the parent is a
> > referenced object. This only applies to file dynptr and skb dynptr,
> > so only pass parent reg->id to kfunc constructors.
> >
> > For release_reference(), invalidating an object now also invalidates
> > all descendants by traversing the object tree. This is done using
> > stack-based DFS to avoid recursive call chains of release_reference() ->
> > unmark_stack_slots_dynptr() -> release_reference(). Referenced objects
> > encountered during tree traversal cannot be indirectly released. They
> > require an explicit helper/kfunc call to release the acquired resources.
> >
> > While the new design changes how object relationships are tracked in
> > the verifier, it does not change the verifier's behavior. Here is the
> > implication for dynptr, pointer casting, and owning/non-owning
> > references:
> >
> > Dynptr:
> >
> > When initializing a dynptr, referenced dynptrs acquire a reference for
> > ref_obj_id. If the dynptr has a referenced parent, parent_id tracks the
> > parent's id. When cloning, ref_obj_id and parent_id are copied from the
> > original. Releasing a referenced dynptr via release_reference(ref_obj_id)
> > invalidates all clones and derived slices. For non-referenced dynptrs,
> > only the specific dynptr and its children are invalidated.
> >
> > Pointer casting:
> >
> > Referenced socket pointers and their casted counterparts share the same
> > lifetime but have different nullness — they have different id but the
> > same ref_obj_id.
> >
> > Owning to non-owning reference conversion:
> >
> > After converting owning to non-owning by clearing ref_obj_id (e.g.,
> > object(id=1, ref_obj_id=1) -> object(id=1, ref_obj_id=0)), the
> > verifier only needs to release the reference state, so it calls
> > release_reference_nomark() instead of release_reference().
> >
> > Note that the error message "reference has not been acquired before" in
> > the helper and kfunc release paths is removed. This message was already
> > unreachable. The verifier only calls release_reference() after
> > confirming meta.ref_obj_id is valid, so the condition could never
> > trigger in practice (no selftest exercises it either). With the
> > refactor, release_reference() can now be called with non-acquired ids
> > and have different error conditions. Report directly in
> > release_reference() instead.
> >
> > Fixes: 870c28588afa ("bpf: net_sched: Add basic bpf qdisc kfuncs")
> > Signed-off-by: Amery Hung <ameryhung@gmail.com>
> > ---
> > include/linux/bpf_verifier.h | 22 ++-
> > kernel/bpf/log.c | 4 +-
> > kernel/bpf/states.c | 9 +-
> > kernel/bpf/verifier.c | 264 +++++++++++++++++------------------
> > 4 files changed, 152 insertions(+), 147 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index dc0cff59246d..1314299c3763 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -65,7 +65,6 @@ struct bpf_reg_state {
> >
> > struct { /* for PTR_TO_MEM | PTR_TO_MEM_OR_NULL */
> > u32 mem_size;
> > - u32 dynptr_id; /* for dynptr slices */
> > };
> >
> > /* For dynptr stack slots */
> > @@ -193,6 +192,13 @@ struct bpf_reg_state {
> > * allowed and has the same effect as bpf_sk_release(sk).
> > */
> > u32 ref_obj_id;
> > + /* Tracks the parent object this register was derived from.
> > + * Used for cascading invalidation: when the parent object is
> > + * released or invalidated, all registers with matching parent_id
> > + * are also invalidated. For example, a slice from bpf_dynptr_data()
> > + * gets parent_id set to the dynptr's id.
> > + */
> > + u32 parent_id;
> > /* Inside the callee two registers can be both PTR_TO_STACK like
> > * R1=fp-8 and R2=fp-8, but one of them points to this function stack
> > * while another to the caller's stack. To differentiate them 'frameno'
> > @@ -508,7 +514,7 @@ struct bpf_verifier_state {
> > iter < frame->allocated_stack / BPF_REG_SIZE; \
> > iter++, reg = bpf_get_spilled_reg(iter, frame, mask))
> >
> > -#define bpf_for_each_reg_in_vstate_mask(__vst, __state, __reg, __mask, __expr) \
> > +#define bpf_for_each_reg_in_vstate_mask(__vst, __state, __reg, __stack, __mask, __expr) \
> > ({ \
> > struct bpf_verifier_state *___vstate = __vst; \
> > int ___i, ___j; \
> > @@ -516,6 +522,7 @@ struct bpf_verifier_state {
> > struct bpf_reg_state *___regs; \
> > __state = ___vstate->frame[___i]; \
> > ___regs = __state->regs; \
> > + __stack = NULL; \
> > for (___j = 0; ___j < MAX_BPF_REG; ___j++) { \
> > __reg = &___regs[___j]; \
> > (void)(__expr); \
> > @@ -523,14 +530,19 @@ struct bpf_verifier_state {
> > bpf_for_each_spilled_reg(___j, __state, __reg, __mask) { \
> > if (!__reg) \
> > continue; \
> > + __stack = &__state->stack[___j]; \
> > (void)(__expr); \
> > } \
> > } \
> > })
> >
> > /* Invoke __expr over regsiters in __vst, setting __state and __reg */
> > -#define bpf_for_each_reg_in_vstate(__vst, __state, __reg, __expr) \
> > - bpf_for_each_reg_in_vstate_mask(__vst, __state, __reg, 1 << STACK_SPILL, __expr)
> > +#define bpf_for_each_reg_in_vstate(__vst, __state, __reg, __expr) \
> > + ({ \
> > + struct bpf_stack_state * ___stack; \
> > + bpf_for_each_reg_in_vstate_mask(__vst, __state, __reg, ___stack,\
> > + 1 << STACK_SPILL, __expr); \
> > + })
> >
> > /* linked list of verifier states used to prune search */
> > struct bpf_verifier_state_list {
> > @@ -1323,6 +1335,7 @@ struct bpf_dynptr_desc {
> > enum bpf_dynptr_type type;
> > u32 id;
> > u32 ref_obj_id;
> > + u32 parent_id;
> > };
> >
> > struct bpf_kfunc_call_arg_meta {
> > @@ -1334,6 +1347,7 @@ struct bpf_kfunc_call_arg_meta {
> > const char *func_name;
> > /* Out parameters */
> > u32 ref_obj_id;
> > + u32 id;
> > u8 release_regno;
> > bool r0_rdonly;
> > u32 ret_btf_id;
> > diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
> > index 011e4ec25acd..d8dd372e45cd 100644
> > --- a/kernel/bpf/log.c
> > +++ b/kernel/bpf/log.c
> > @@ -667,6 +667,8 @@ static void print_reg_state(struct bpf_verifier_env *env,
> > verbose(env, "%+d", reg->delta);
> > if (reg->ref_obj_id)
> > verbose_a("ref_obj_id=%d", reg->ref_obj_id);
> > + if (reg->parent_id)
> > + verbose_a("parent_id=%d", reg->parent_id);
> > if (type_is_non_owning_ref(reg->type))
> > verbose_a("%s", "non_own_ref");
> > if (type_is_map_ptr(t)) {
> > @@ -770,8 +772,6 @@ void print_verifier_state(struct bpf_verifier_env *env, const struct bpf_verifie
> > verbose_a("id=%d", reg->id);
> > if (reg->ref_obj_id)
> > verbose_a("ref_id=%d", reg->ref_obj_id);
> > - if (reg->dynptr_id)
> > - verbose_a("dynptr_id=%d", reg->dynptr_id);
> > verbose(env, ")");
> > break;
> > case STACK_ITER:
> > diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
> > index 8478d2c6ed5b..72bd3bcda5fb 100644
> > --- a/kernel/bpf/states.c
> > +++ b/kernel/bpf/states.c
> > @@ -494,7 +494,8 @@ static bool regs_exact(const struct bpf_reg_state *rold,
> > {
> > return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&
> > check_ids(rold->id, rcur->id, idmap) &&
> > - check_ids(rold->ref_obj_id, rcur->ref_obj_id, idmap);
> > + check_ids(rold->ref_obj_id, rcur->ref_obj_id, idmap) &&
> > + check_ids(rold->parent_id, rcur->parent_id, idmap);
> > }
> >
> > enum exact_level {
> > @@ -619,7 +620,8 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold,
> > range_within(rold, rcur) &&
> > tnum_in(rold->var_off, rcur->var_off) &&
> > check_ids(rold->id, rcur->id, idmap) &&
> > - check_ids(rold->ref_obj_id, rcur->ref_obj_id, idmap);
> > + check_ids(rold->ref_obj_id, rcur->ref_obj_id, idmap) &&
> > + check_ids(rold->parent_id, rcur->parent_id, idmap);
> > case PTR_TO_PACKET_META:
> > case PTR_TO_PACKET:
> > /* We must have at least as much range as the old ptr
> > @@ -799,7 +801,8 @@ static bool stacksafe(struct bpf_verifier_env *env, struct bpf_func_state *old,
> > cur_reg = &cur->stack[spi].spilled_ptr;
> > if (old_reg->dynptr.type != cur_reg->dynptr.type ||
> > old_reg->dynptr.first_slot != cur_reg->dynptr.first_slot ||
> > - !check_ids(old_reg->ref_obj_id, cur_reg->ref_obj_id, idmap))
> > + !check_ids(old_reg->ref_obj_id, cur_reg->ref_obj_id, idmap) ||
> > + !check_ids(old_reg->parent_id, cur_reg->parent_id, idmap))
> > return false;
> > break;
> > case STACK_ITER:
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 0313b7d5f6c9..908a3af0e7c4 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -201,7 +201,7 @@ struct bpf_verifier_stack_elem {
> >
> > static int acquire_reference(struct bpf_verifier_env *env, int insn_idx);
> > static int release_reference_nomark(struct bpf_verifier_state *state, int ref_obj_id);
> > -static int release_reference(struct bpf_verifier_env *env, int ref_obj_id);
> > +static int release_reference(struct bpf_verifier_env *env, int id);
> > static void invalidate_non_owning_refs(struct bpf_verifier_env *env);
> > static bool in_rbtree_lock_required_cb(struct bpf_verifier_env *env);
> > static int ref_set_non_owning(struct bpf_verifier_env *env,
> > @@ -241,6 +241,7 @@ struct bpf_call_arg_meta {
> > int mem_size;
> > u64 msize_max_value;
> > int ref_obj_id;
> > + u32 id;
> > int func_id;
> > struct btf *btf;
> > u32 btf_id;
> > @@ -603,14 +604,14 @@ static enum bpf_type_flag get_dynptr_type_flag(enum bpf_dynptr_type type)
> > }
> > }
> >
> > -static bool dynptr_type_refcounted(enum bpf_dynptr_type type)
> > +static bool dynptr_type_referenced(enum bpf_dynptr_type type)
> > {
> > return type == BPF_DYNPTR_TYPE_RINGBUF || type == BPF_DYNPTR_TYPE_FILE;
> > }
> >
> > static void __mark_dynptr_reg(struct bpf_reg_state *reg,
> > enum bpf_dynptr_type type,
> > - bool first_slot, int dynptr_id);
> > + bool first_slot, int id);
> >
> >
> > static void mark_dynptr_stack_regs(struct bpf_verifier_env *env,
> > @@ -635,11 +636,12 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> > struct bpf_func_state *state, int spi);
> >
> > static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> > - enum bpf_arg_type arg_type, int insn_idx, int clone_ref_obj_id)
> > + enum bpf_arg_type arg_type, int insn_idx, int parent_id,
> > + struct bpf_dynptr_desc *dynptr)
> > {
> > struct bpf_func_state *state = bpf_func(env, reg);
> > + int spi, i, err, ref_obj_id = 0;
> > enum bpf_dynptr_type type;
> > - int spi, i, err;
> >
> > spi = dynptr_get_spi(env, reg);
> > if (spi < 0)
> > @@ -673,82 +675,56 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> > mark_dynptr_stack_regs(env, &state->stack[spi].spilled_ptr,
> > &state->stack[spi - 1].spilled_ptr, type);
> >
> > - if (dynptr_type_refcounted(type)) {
> > - /* The id is used to track proper releasing */
> > - int id;
> > -
> > - if (clone_ref_obj_id)
> > - id = clone_ref_obj_id;
> > - else
> > - id = acquire_reference(env, insn_idx);
> > -
> > - if (id < 0)
> > - return id;
> > -
> > - state->stack[spi].spilled_ptr.ref_obj_id = id;
> > - state->stack[spi - 1].spilled_ptr.ref_obj_id = id;
> > + if (dynptr->type == BPF_DYNPTR_TYPE_INVALID) { /* dynptr constructors */
> > + if (dynptr_type_referenced(type)) {
> > + ref_obj_id = acquire_reference(env, insn_idx);
> > + if (ref_obj_id < 0)
> > + return ref_obj_id;
> > + }
> > + } else { /* bpf_dynptr_clone() */
> > + ref_obj_id = dynptr->ref_obj_id;
> > + parent_id = dynptr->parent_id;
> > }
> >
> > + state->stack[spi].spilled_ptr.ref_obj_id = ref_obj_id;
> > + state->stack[spi - 1].spilled_ptr.ref_obj_id = ref_obj_id;
> > + state->stack[spi].spilled_ptr.parent_id = parent_id;
> > + state->stack[spi - 1].spilled_ptr.parent_id = parent_id;
> > +
> > return 0;
> > }
> >
> > -static void invalidate_dynptr(struct bpf_verifier_env *env, struct bpf_func_state *state, int spi)
> > +static void invalidate_dynptr(struct bpf_verifier_env *env, struct bpf_func_state *state,
>
> it looks like state is no longer needed.
Ack.
>
> > + struct bpf_stack_state *stack)
> > {
> > int i;
> >
> > for (i = 0; i < BPF_REG_SIZE; i++) {
> > - state->stack[spi].slot_type[i] = STACK_INVALID;
> > - state->stack[spi - 1].slot_type[i] = STACK_INVALID;
> > + stack[0].slot_type[i] = STACK_INVALID;
> > + stack[1].slot_type[i] = STACK_INVALID;
> > }
> >
> > - bpf_mark_reg_not_init(env, &state->stack[spi].spilled_ptr);
> > - bpf_mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr);
> > + bpf_mark_reg_not_init(env, &stack[0].spilled_ptr);
> > + bpf_mark_reg_not_init(env, &stack[1].spilled_ptr);
> > }
> >
> > static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > {
> > struct bpf_func_state *state = bpf_func(env, reg);
> > - int spi, ref_obj_id, i;
> > + int spi;
> >
> > spi = dynptr_get_spi(env, reg);
> > if (spi < 0)
> > return spi;
> >
> > - if (!dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
> > - invalidate_dynptr(env, state, spi);
> > - return 0;
> > - }
> > -
> > - ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
> > -
> > - /* If the dynptr has a ref_obj_id, then we need to invalidate
> > - * two things:
> > - *
> > - * 1) Any dynptrs with a matching ref_obj_id (clones)
> > - * 2) Any slices derived from this dynptr.
> > + /*
> > + * For referenced dynptr, the clones share the same ref_obj_id and will be
> > + * invalidated too. For non-referenced dynptr, only the dynptr and slices
> > + * derived from it will be invalidated.
> > */
> > -
> > - /* Invalidate any slices associated with this dynptr */
> > - WARN_ON_ONCE(release_reference(env, ref_obj_id));
> > -
> > - /* Invalidate any dynptr clones */
> > - for (i = 1; i < state->allocated_stack / BPF_REG_SIZE; i++) {
> > - if (state->stack[i].spilled_ptr.ref_obj_id != ref_obj_id)
> > - continue;
> > -
> > - /* it should always be the case that if the ref obj id
> > - * matches then the stack slot also belongs to a
> > - * dynptr
> > - */
> > - if (state->stack[i].slot_type[0] != STACK_DYNPTR) {
> > - verifier_bug(env, "misconfigured ref_obj_id");
> > - return -EFAULT;
> > - }
> > - if (state->stack[i].spilled_ptr.dynptr.first_slot)
> > - invalidate_dynptr(env, state, i);
> > - }
> > -
> > - return 0;
> > + reg = &state->stack[spi].spilled_ptr;
> > + return release_reference(env, dynptr_type_referenced(reg->dynptr.type) ?
> > + reg->ref_obj_id : reg->id);
> > }
> >
> > static void __mark_reg_unknown(const struct bpf_verifier_env *env,
> > @@ -765,10 +741,6 @@ static void mark_reg_invalid(const struct bpf_verifier_env *env, struct bpf_reg_
> > static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> > struct bpf_func_state *state, int spi)
> > {
> > - struct bpf_func_state *fstate;
> > - struct bpf_reg_state *dreg;
> > - int i, dynptr_id;
> > -
> > /* We always ensure that STACK_DYNPTR is never set partially,
> > * hence just checking for slot_type[0] is enough. This is
> > * different for STACK_SPILL, where it may be only set for
> > @@ -781,9 +753,9 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> > if (!state->stack[spi].spilled_ptr.dynptr.first_slot)
> > spi = spi + 1;
> >
> > - if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) {
> > + if (dynptr_type_referenced(state->stack[spi].spilled_ptr.dynptr.type)) {
> > int ref_obj_id = state->stack[spi].spilled_ptr.ref_obj_id;
> > - int ref_cnt = 0;
> > + int i, ref_cnt = 0;
> >
> > /*
> > * A referenced dynptr can be overwritten only if there is at
> > @@ -808,29 +780,8 @@ static int destroy_if_dynptr_stack_slot(struct bpf_verifier_env *env,
> > mark_stack_slot_scratched(env, spi);
> > mark_stack_slot_scratched(env, spi - 1);
> >
> > - /* Writing partially to one dynptr stack slot destroys both. */
> > - for (i = 0; i < BPF_REG_SIZE; i++) {
> > - state->stack[spi].slot_type[i] = STACK_INVALID;
> > - state->stack[spi - 1].slot_type[i] = STACK_INVALID;
> > - }
> > -
> > - dynptr_id = state->stack[spi].spilled_ptr.id;
> > - /* Invalidate any slices associated with this dynptr */
> > - bpf_for_each_reg_in_vstate(env->cur_state, fstate, dreg, ({
> > - /* Dynptr slices are only PTR_TO_MEM_OR_NULL and PTR_TO_MEM */
> > - if (dreg->type != (PTR_TO_MEM | PTR_MAYBE_NULL) && dreg->type != PTR_TO_MEM)
> > - continue;
> > - if (dreg->dynptr_id == dynptr_id)
> > - mark_reg_invalid(env, dreg);
> > - }));
> > -
> > - /* Do not release reference state, we are destroying dynptr on stack,
> > - * not using some helper to release it. Just reset register.
> > - */
> > - bpf_mark_reg_not_init(env, &state->stack[spi].spilled_ptr);
> > - bpf_mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr);
> > -
> > - return 0;
> > + /* Invalidate the dynptr and any derived slices */
> > + return release_reference(env, state->stack[spi].spilled_ptr.id);
> > }
> >
> > static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > @@ -1449,15 +1400,15 @@ static void release_reference_state(struct bpf_verifier_state *state, int idx)
> > return;
> > }
> >
> > -static bool find_reference_state(struct bpf_verifier_state *state, int ptr_id)
> > +static struct bpf_reference_state *find_reference_state(struct bpf_verifier_state *state, int ptr_id)
> > {
> > int i;
> >
> > for (i = 0; i < state->acquired_refs; i++)
> > if (state->refs[i].id == ptr_id)
> > - return true;
> > + return &state->refs[i];
> >
> > - return false;
> > + return NULL;
> > }
> >
> > static int release_lock_state(struct bpf_verifier_state *state, int type, int id, void *ptr)
> > @@ -1764,6 +1715,7 @@ static void __mark_reg_known(struct bpf_reg_state *reg, u64 imm)
> > offsetof(struct bpf_reg_state, var_off) - sizeof(reg->type));
> > reg->id = 0;
> > reg->ref_obj_id = 0;
> > + reg->parent_id = 0;
> > ___mark_reg_known(reg, imm);
> > }
> >
> > @@ -1801,7 +1753,7 @@ static void mark_reg_known_zero(struct bpf_verifier_env *env,
> > }
> >
> > static void __mark_dynptr_reg(struct bpf_reg_state *reg, enum bpf_dynptr_type type,
> > - bool first_slot, int dynptr_id)
> > + bool first_slot, int id)
> > {
> > /* reg->type has no meaning for STACK_DYNPTR, but when we set reg for
> > * callback arguments, it does need to be CONST_PTR_TO_DYNPTR, so simply
> > @@ -1810,7 +1762,7 @@ static void __mark_dynptr_reg(struct bpf_reg_state *reg, enum bpf_dynptr_type ty
> > __mark_reg_known_zero(reg);
> > reg->type = CONST_PTR_TO_DYNPTR;
> > /* Give each dynptr a unique id to uniquely associate slices to it. */
> > - reg->id = dynptr_id;
> > + reg->id = id;
> > reg->dynptr.type = type;
> > reg->dynptr.first_slot = first_slot;
> > }
> > @@ -2451,6 +2403,7 @@ void bpf_mark_reg_unknown_imprecise(struct bpf_reg_state *reg)
> > reg->type = SCALAR_VALUE;
> > reg->id = 0;
> > reg->ref_obj_id = 0;
> > + reg->parent_id = 0;
>
> nit: it looks like we can do memset(0) on the entire reg and then set
> fields that need to be non-zero: type, var_off + __mark_reg_unbounded().
>
> > reg->var_off = tnum_unknown;
> > reg->frameno = 0;
> > reg->precise = false;
> > @@ -7427,7 +7380,7 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno,
> > * and checked dynamically during runtime.
> > */
> > static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn_idx,
> > - enum bpf_arg_type arg_type, int clone_ref_obj_id,
> > + enum bpf_arg_type arg_type, int parent_id,
> > struct bpf_dynptr_desc *dynptr)
> > {
> > struct bpf_reg_state *reg = reg_state(env, regno);
> > @@ -7470,7 +7423,8 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn
> > return err;
> > }
> >
> > - err = mark_stack_slots_dynptr(env, reg, arg_type, insn_idx, clone_ref_obj_id);
> > + err = mark_stack_slots_dynptr(env, reg, arg_type, insn_idx, parent_id,
> > + dynptr);
> > } else /* OBJ_RELEASE and None case from above */ {
> > /* For the reg->type == PTR_TO_STACK case, bpf_dynptr is never const */
> > if (reg->type == CONST_PTR_TO_DYNPTR && (arg_type & OBJ_RELEASE)) {
> > @@ -7507,6 +7461,7 @@ static int process_dynptr_func(struct bpf_verifier_env *env, int regno, int insn
> > dynptr->id = reg->id;
> > dynptr->type = reg->dynptr.type;
> > dynptr->ref_obj_id = reg->ref_obj_id;
> > + dynptr->parent_id = reg->parent_id;
> > }
> > }
> > return err;
> > @@ -8461,7 +8416,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > */
> > if (reg->type == PTR_TO_STACK) {
> > spi = dynptr_get_spi(env, reg);
> > - if (spi < 0 || !state->stack[spi].spilled_ptr.ref_obj_id) {
> > + if (spi < 0 || !state->stack[spi].spilled_ptr.id) {
> > verbose(env, "arg %d is an unacquired reference\n", regno);
> > return -EINVAL;
> > }
> > @@ -8489,6 +8444,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> > return -EACCES;
> > }
> > meta->ref_obj_id = reg->ref_obj_id;
> > + meta->id = reg->id;
> > }
> >
> > switch (base_type(arg_type)) {
> > @@ -9111,26 +9067,75 @@ static int release_reference_nomark(struct bpf_verifier_state *state, int ref_ob
> > return -EINVAL;
> > }
> >
> > -/* The pointer with the specified id has released its reference to kernel
> > - * resources. Identify all copies of the same pointer and clear the reference.
> > - *
> > - * This is the release function corresponding to acquire_reference(). Idempotent.
> > - */
> > -static int release_reference(struct bpf_verifier_env *env, int ref_obj_id)
> > +static int idstack_push(struct bpf_idmap *idmap, u32 id)
> > +{
> > + int i;
> > +
> > + if (!id)
> > + return 0;
> > +
> > + for (i = 0; i < idmap->cnt; i++)
> > + if (idmap->map[i].old == id)
> > + return 0;
> > +
> > + if (WARN_ON_ONCE(idmap->cnt >= BPF_ID_MAP_SIZE))
> > + return -EFAULT;
>
> It feels like this check belongs above, maybe the first thing to do.
Shouldn't it allow the verification to continue when pushing an id
that already exists in the stack?
>
> > +
> > + idmap->map[idmap->cnt++].old = id;
> > + return 0;
> > +}
> > +
> > +static int idstack_pop(struct bpf_idmap *idmap)
> > {
> > + if (!idmap->cnt)
> > + return 0;
> > +
> > + return idmap->map[--idmap->cnt].old;
> > +}
> > +
> > +/* Release id and objects referencing the id iteratively in a DFS manner */
> > +static int release_reference(struct bpf_verifier_env *env, int id)
> > +{
> > + u32 mask = (1 << STACK_SPILL) | (1 << STACK_DYNPTR);
> > struct bpf_verifier_state *vstate = env->cur_state;
> > + struct bpf_idmap *idstack = &env->idmap_scratch;
> > + struct bpf_stack_state *stack;
> > struct bpf_func_state *state;
> > struct bpf_reg_state *reg;
> > - int err;
> > + int root_id = id, err;
> >
> > - err = release_reference_nomark(vstate, ref_obj_id);
> > - if (err)
> > - return err;
> > + idstack->cnt = 0;
> > + idstack_push(idstack, id);
> >
> > - bpf_for_each_reg_in_vstate(vstate, state, reg, ({
> > - if (reg->ref_obj_id == ref_obj_id)
> > - mark_reg_invalid(env, reg);
> > - }));
> > + if (find_reference_state(vstate, id))
> > + WARN_ON_ONCE(release_reference_nomark(vstate, id));
> > +
> > + while ((id = idstack_pop(idstack))) {
> > + bpf_for_each_reg_in_vstate_mask(vstate, state, reg, stack, mask, ({
> > + if (reg->id != id && reg->parent_id != id && reg->ref_obj_id != id)
> > + continue;
> > +
> > + if (reg->ref_obj_id && id != root_id) {
>
> Does this line check that the only ref_obj_id we should see is either
> 0 or root_id? can we rewrite it as
> if (reg->ref_obj_id && reg->ref_obj_id != root_id)
>
> this is simpler, because id can also be reg->id/reg->parent_id, which is
> hard to reason what that means.
>
Looking at it again, I think my release_reference() is wrong. It first
calls release_reference_nomark() if the id is a ref_obj_id. This will
invalidate all registers with the same ref_obj_id without traversing
their children.
I should remember the root_ref_obj_id instead of calling
release_reference_nomark(). Then, the check you mentioned should be:
if (reg->ref_obj_id && reg->ref_obj_id != root_ref_obj_id)
I will write a selftest to confirm the issue and include it in the
next respin if this turns out to be a bug.
^ permalink raw reply
* Re: [PATCH v2] usb: rtl8150: avoid using uninitialized CSCR value
From: Jakub Kicinski @ 2026-04-23 18:42 UTC (permalink / raw)
To: zhangdandan
Cc: Jakub Kicinski, syzbot+9db6c624635564ad813c, petkan,
andrew+netdev, andrew, michal.pecio, netdev, davem, linux-kernel,
pabeni, horms, linux-usb, edumazet
In-Reply-To: <F20002D619544E87+20260421105145.11394-1-zhangdandan@uniontech.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.
For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
usb: rtl8150: avoid using uninitialized CSCR value
This commit fixes an uninitialized variable bug in set_carrier() where
get_registers() could fail, leaving tmp uninitialized. The fix adds a
return value check to bail out on error, preserving the existing carrier
state.
Reported-by: syzbot+9db6c624635564ad813c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9db6c624635564ad813c
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> set_carrier() reads CSCR via get_registers() (a USB control transfer)
> without checking the return value, so a transient control transfer
> failure would leave the on-stack "tmp" uninitialized and then be used
> to decide the netif carrier state.
>
> Check the return value of get_registers() and bail out on error,
> leaving the previously known carrier state untouched. A failed USB
> control transfer here may be transient (e.g. EMI, flaky cable, retries
> exhausted), so it is wrong to force the link down on such failures and
> cause the carrier state to toggle unnecessarily.
The commit message says "it is wrong to force the link down on such
failures" - but is this characterization accurate?
The original (pre-patch) code did not deliberately force the link down on
failure. It used an uninitialized value, which would randomly either set
carrier on or off depending on stack contents. The statement reads as if
the original code forced link down, but the original bug was random
behavior from uninitialized data.
The "force the link down" language more accurately describes what the
rejected v1 patch did, and why v2 changed the approach to just returning.
>
> This only addresses the uninitialized-value usage and does not change
> link-state policy.
Is this description fully accurate? The patch does change observable
behavior:
Before: on get_registers() failure, carrier state was set to a random value
(on or off based on stack contents).
After: on failure, carrier state is preserved unchanged.
This is a change from "set carrier based on garbage data" to "preserve
previous carrier state." While "using uninitialized data" was never an
intentional policy, the commit does introduce a new explicit behavior
(early return preserving state) that did not exist before.
Additionally, set_carrier() is only called from rtl8150_open() in this
driver, meaning the "previously known carrier state" at that point is the
default state from alloc_etherdev (carrier ON, since __LINK_STATE_NOCARRIER
bit is 0 in zero-initialized memory). On first open, a get_registers()
failure would preserve carrier=ON, which could report link up when the
register read failed.
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 4cda0643afb6..76962dcd127d 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -722,7 +722,8 @@ static void set_carrier(struct net_device *netdev)
> rtl8150_t *dev = netdev_priv(netdev);
> short tmp;
>
> - get_registers(dev, CSCR, 2, &tmp);
> + if (get_registers(dev, CSCR, 2, &tmp) < 0)
> + return;
> if (tmp & CSCR_LINK_STATUS)
> netif_carrier_on(netdev);
> else
--
pw-bot: cr
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH iwl-net v2] idpf: do not perform flow ops when netdev is detached
From: Jacob Keller @ 2026-04-23 18:40 UTC (permalink / raw)
To: Simon Horman, boolli
Cc: anthony.l.nguyen, przemyslaw.kitszel, davem, kuba, edumazet,
intel-wired-lan, netdev, linux-kernel, decot, anjali.singhai,
sridhar.samudrala, brianvv, emil.s.tantilov, stable
In-Reply-To: <20260423163307.989421-3-horms@kernel.org>
On 4/23/2026 9:33 AM, Simon Horman wrote:
> From: 'Simon Horman' <horms@kernel.org>
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> idpf: do not perform flow ops when netdev is detached
>
> This commit adds checks to prevent crashes when out-of-tree modules bypass
> ethtool core locks and call flow operations on detached netdevs during a
> hardware reset. It introduces netdev and netif_device_present checks in
> idpf_get_rxnfc and idpf_set_rxnfc.
>
>> Even though commit 2e281e1155fc ("idpf: detach and close netdevs while
>> handling a reset") prevents ethtool -N/-n operations to operate on
>> detached netdevs, we found that out-of-tree workflows like OpenOnload
>> can bypass ethtool core locks and call idpf_set_rxnfc directly during
>> an idpf HW reset.
>
> Is it acceptable to add defensive checks upstream solely to accommodate
> out-of-tree modules that bypass core kernel locking?
>
> The core ethtool infrastructure manages locks like rtnl_lock for in-tree
> callers to guarantee safety. Modifying the upstream driver to paper over
> locking violations in out-of-tree modules might contradict upstream policies.
>
> Simon says: If this is to address OOT module behaviour then I am concerned.
> And I would suggest that the correct approach is to fix
> the OOT kernel module.
>
The commit message says: "OpenOnload can bypass ethtool core locks and
call idpf_set_rxnfc directly". But if it somehow calls
idpf_set_rxnfc_directly without holding the appropriate locks, then no
amount of changes to the idpf driver will fix that issue. It is simply
unsafe for that to be occurring, and this feels like it just patches one
specific problem without addressing the root cause that something is
calling the drivers ethtool function without correctly holding the
expected locks.
@Li Li, could you please explain more details about the workflow that
triggers these behaviors? If it can't be reproduced with in-tree modules
then I don't think we can accept this fix.
^ permalink raw reply
* [PATCH bpf v2] bpf: Fix NULL pointer dereference in bpf_skb_fib_lookup()
From: Weiming Shi @ 2026-04-23 18:38 UTC (permalink / raw)
To: Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
Andrii Nakryiko, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: John Fastabend, Stanislav Fomichev, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Hao Luo, Jiri Olsa, Simon Horman,
Jesper Dangaard Brouer, bpf, netdev, Xiang Mei, Weiming Shi,
Paul Chaignon
When tot_len is not provided by the user, bpf_skb_fib_lookup()
resolves the FIB result's output device via dev_get_by_index_rcu()
to check skb forwardability and fill in mtu_result. The returned
pointer is dereferenced without a NULL check. If the device is
concurrently unregistered, dev_get_by_index_rcu() returns NULL and
is_skb_forwardable() crashes at dev->flags:
KASAN: null-ptr-deref in range
[0x00000000000000b0-0x00000000000000b7]
Call Trace:
is_skb_forwardable (include/linux/netdevice.h:4365)
bpf_skb_fib_lookup (net/core/filter.c:6446)
bpf_prog_test_run_skb (net/bpf/test_run.c)
__sys_bpf (kernel/bpf/syscall.c)
Add the missing NULL check, returning -ENODEV to be consistent
with how bpf_ipv4_fib_lookup() and bpf_ipv6_fib_lookup() handle
the same condition.
Fixes: 4f74fede40df ("bpf: Add mtu checking to FIB forwarding helper")
Reported-by: Xiang Mei <xmei5@asu.edu>
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
Acked-by: Paul Chaignon <paul.chaignon@gmail.com>
---
v2:
Fix Fixes tag: 4f74fede40df, not e1850ea9bd9e (Jiayuan Chen)
Add unlikely() to match bpf_ipv{4,6}_fib_lookup() style (Paul Chaignon)
net/core/filter.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/filter.c b/net/core/filter.c
index 78b548158fb0..5b4aa9faa707 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6450,6 +6450,8 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb,
* against MTU of FIB lookup resulting net_device
*/
dev = dev_get_by_index_rcu(net, params->ifindex);
+ if (unlikely(!dev))
+ return -ENODEV;
if (!is_skb_forwardable(dev, skb))
rc = BPF_FIB_LKUP_RET_FRAG_NEEDED;
--
2.43.0
^ permalink raw reply related
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