* Re: [PATCH net-next v6 3/7] net: macb: Add page pool support handle multi-descriptor frame rx
From: Jakub Kicinski @ 2026-03-29 19:47 UTC (permalink / raw)
To: pvalerio
Cc: Jakub Kicinski, netdev, nicolas.ferre, claudiu.beznea,
andrew+netdev, davem, edumazet, pabeni, lorenzo, theo.lebrun, nb
In-Reply-To: <20260323221047.2749577-4-pvalerio@redhat.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.
---
Subject: net: macb: Add page pool support handle multi-descriptor frame rx
This patch adds page pool allocator and skb recycling support for the data
buffers in the Cadence MACB/GEM Ethernet driver. It also adds support for
receiving network frames that span multiple DMA descriptors.
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
> +static unsigned int gem_total_rx_buffer_size(struct macb *bp)
> +{
> + return SKB_HEAD_ALIGN(bp->rx_buffer_size + NET_SKB_PAD);
> +}
When calculating the size for napi_build_skb(), does this code account
for the skb_shared_info overhead?
napi_build_skb() places the shared info at the end of the provided fragment
size (frag_size - sizeof(skb_shared_info)). Without adding
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) to the total size, the shared
info could overlap with the DMA buffer space. This could overwrite packet
data and cause an skb_over_panic() on skb_put() for large frames.
[ ... ]
> @@ -1500,25 +1543,26 @@ static int gem_rx_refill(struct macb_queue *queue)
> desc = macb_rx_desc(queue, entry);
>
> if (!queue->rx_buff[entry]) {
> - /* allocate sk_buff for this free entry in ring */
> - skb = netdev_alloc_skb(bp->dev, bp->rx_buffer_size);
> - if (unlikely(!skb)) {
> - netdev_err(bp->dev,
> - "Unable to allocate sk_buff\n");
> + gfp_alloc = napi ? GFP_ATOMIC : GFP_KERNEL;
> + page = page_pool_alloc_frag(queue->page_pool, &offset,
> + gem_total_rx_buffer_size(bp),
> + gfp_alloc | __GFP_NOWARN);
Can gem_rx_refill() be called concurrently by both the NAPI poll loop and
the error recovery workqueue?
The page pool allocator uses a lockless fast-path that requires execution
from a single context. Looking at the driver, macb_hresp_error_task() is a
workqueue that calls gem_init_rings() which in turn calls gem_rx_refill().
If the workqueue path calls gem_rx_refill() without first stopping NAPI
(via napi_disable()), does this risk concurrent access and memory corruption
in the page pool ring?
[ ... ]
> @@ -2600,12 +2700,22 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
> return ret;
> }
>
> -static void macb_init_rx_buffer_size(struct macb *bp, size_t size)
> +static void macb_init_rx_buffer_size(struct macb *bp, unsigned int mtu)
> {
> + unsigned int overhead;
> + size_t size;
> +
> if (!macb_is_gem(bp)) {
> bp->rx_buffer_size = MACB_RX_BUFFER_SIZE;
> } else {
> - bp->rx_buffer_size = MIN(size, RX_BUFFER_MAX);
> + size = mtu + ETH_HLEN + ETH_FCS_LEN;
> + bp->rx_buffer_size = SKB_DATA_ALIGN(size + bp->rx_ip_align);
> + if (gem_total_rx_buffer_size(bp) > PAGE_SIZE) {
> + overhead = bp->rx_headroom +
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> + bp->rx_buffer_size = rounddown(PAGE_SIZE - overhead,
> + RX_BUFFER_MULTIPLE);
> + }
>
> if (bp->rx_buffer_size % RX_BUFFER_MULTIPLE) {
Could this calculation result in a page pool allocation request that
exceeds PAGE_SIZE?
Because the check for gem_total_rx_buffer_size(bp) > PAGE_SIZE happens
before bp->rx_buffer_size is rounded up to RX_BUFFER_MULTIPLE (which occurs
just below this block), specific MTU values might pass the initial check but
exceed PAGE_SIZE after rounding.
Since the page pool is initialized with order = 0, requests larger than a
page will unconditionally fail.
^ permalink raw reply
* Re: [PATCH net-next v6 4/7] net: macb: make macb_tx_skb generic
From: Jakub Kicinski @ 2026-03-29 19:47 UTC (permalink / raw)
To: pvalerio
Cc: Jakub Kicinski, netdev, nicolas.ferre, claudiu.beznea,
andrew+netdev, davem, edumazet, pabeni, lorenzo, theo.lebrun, nb
In-Reply-To: <20260323221047.2749577-5-pvalerio@redhat.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: macb: make macb_tx_skb generic
This commit renames the macb_tx_skb structure to macb_tx_buff with no
functional changes. This is done as a preparatory step for adding XDP
xmit support.
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
> @@ -2404,12 +2404,12 @@ static unsigned int macb_tx_map(struct macb *bp,
> }
>
> /* Should never happen */
> - if (unlikely(!tx_skb)) {
> + if (unlikely(!tx_buff)) {
> netdev_err(bp->dev, "BUG! empty skb!\n");
> return 0;
> }
Does returning 0 here cause a memory leak?
If macb_tx_map() returns 0 for an empty skb, the caller macb_start_xmit()
will assume the packet was successfully enqueued and will not free it. Should
this return an error code like -EINVAL instead to trigger the
dev_kfree_skb_any() error path?
[ ... ]
> @@ -2808,8 +2808,8 @@ static void macb_free_consistent(struct macb *bp)
> dma_free_coherent(dev, size, bp->queues[0].rx_ring, bp->queues[0].rx_ring_dma);
>
> for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> - kfree(queue->tx_skb);
> - queue->tx_skb = NULL;
> + kfree(queue->tx_buff);
> + queue->tx_buff = NULL;
Is queue->tx_error_task guaranteed to be canceled before we free
queue->tx_buff here?
Since macb_close() calls macb_free_consistent() without explicitly calling
cancel_work_sync(&queue->tx_error_task), could a pending background error
task execute and access the freed array, resulting in a use-after-free?
[ ... ]
> @@ -2887,9 +2887,9 @@ static int macb_alloc_consistent(struct macb *bp)
> queue->rx_ring = rx + macb_rx_ring_size_per_queue(bp) * q;
> queue->rx_ring_dma = rx_dma + macb_rx_ring_size_per_queue(bp) * q;
>
> - size = bp->tx_ring_size * sizeof(struct macb_tx_skb);
> - queue->tx_skb = kmalloc(size, GFP_KERNEL);
> - if (!queue->tx_skb)
> + size = bp->tx_ring_size * sizeof(struct macb_tx_buff);
> + queue->tx_buff = kmalloc(size, GFP_KERNEL);
> + if (!queue->tx_buff)
> goto out_err;
> }
This isn't a bug, but would it be better to use kcalloc() or kmalloc_array()
here instead of manual size multiplication?
^ permalink raw reply
* Re: [PATCH net-next v6 5/7] net: macb: generalize tx buffer handling
From: Jakub Kicinski @ 2026-03-29 19:47 UTC (permalink / raw)
To: pvalerio
Cc: Jakub Kicinski, netdev, nicolas.ferre, claudiu.beznea,
andrew+netdev, davem, edumazet, pabeni, lorenzo, theo.lebrun, nb
In-Reply-To: <20260323221047.2749577-6-pvalerio@redhat.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: macb: generalize tx buffer handling
This commit renames the skb member in struct macb_tx_buff to ptr and
introduces an enum macb_tx_buff_type. This prepares the driver for XDP xmit
support by allowing the buffer to handle both sk_buff and xdp frames.
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a71d36b18170..4c5dd70f003b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1197,7 +1197,8 @@ static int macb_halt_tx(struct macb *bp)
> bp, TSR);
> }
>
> -static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff, int budget)
> +static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff,
> + int budget)
> {
[ ... ]
> - if (tx_buff->skb) {
> - napi_consume_skb(tx_buff->skb, budget);
> - tx_buff->skb = NULL;
> + if (tx_buff->ptr) {
> + napi_consume_skb(tx_buff->ptr, budget);
> + tx_buff->ptr = NULL;
> }
> }
Does the teardown path iterate over the uncompleted descriptors to call
macb_tx_unmap()?
When the interface is closed, the hardware is reset and macb_free_consistent()
frees the tx_buff array directly via kfree(queue->tx_buff).
If the teardown path does not iterate from tx_tail to tx_head, will any
packets (and future xdp frames) that are currently in the transmit ring when
the interface is brought down be permanently leaked, along with their
associated DMA mappings?
[ ... ]
> @@ -1298,7 +1299,7 @@ static void macb_tx_error_task(struct work_struct *work)
> desc = macb_tx_desc(queue, tail);
> ctrl = desc->ctrl;
> tx_buff = macb_tx_buff(queue, tail);
> - skb = tx_buff->skb;
> + skb = tx_buff->ptr;
Is the tx_error_task workqueue canceled before freeing tx_buff during device
teardown?
The macb_tx_error_task background task accesses the queue->tx_buff array and
the hardware tx_ring. During device teardown, macb_free_consistent() frees
queue->tx_buff using kfree().
If cancel_work_sync(&queue->tx_error_task) is not called in the teardown
path, could a transmit error schedule this workqueue just before the device
is brought down, causing the task to execute after the structures have been
destroyed and resulting in a use-after-free?
[ ... ]
> @@ -2365,7 +2368,8 @@ static unsigned int macb_tx_map(struct macb *bp,
> goto dma_error;
>
> /* Save info to properly release resources */
> - tx_buff->skb = NULL;
> + tx_buff->ptr = NULL;
> + tx_buff->type = MACB_TYPE_SKB;
What happens if this function receives an empty skb and fails to find any
fragments?
Looking at the rest of macb_tx_map(), if the packet has no data length, it
appears the function logs an error and returns 0:
/* Should never happen */
if (unlikely(!tx_buff)) {
netdev_err(bp->dev, "BUG! empty skb!\n");
return 0;
}
Since the caller treats 0 as success, it would skip the error path that frees
the skb. As no descriptor was actually queued and tx_head wasn't updated, will
the packet never be unmapped or freed by the completion path, causing a
permanent memory leak? Should this return a non-zero error code instead?
[ ... ]
> @@ -5283,8 +5289,9 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
> netif_stop_queue(dev);
>
> /* Store packet information (to free when Tx completed) */
> - lp->rm9200_txq[desc].skb = skb;
> + lp->rm9200_txq[desc].ptr = skb;
> lp->rm9200_txq[desc].size = skb->len;
> + lp->rm9200_txq[desc].type = MACB_TYPE_SKB;
> lp->rm9200_txq[desc].mapping = dma_map_single(&lp->pdev->dev, skb->data,
Can a DMA mapping error here cause a permanent queue hang?
If dma_mapping_error() fails just below this, the driver calls
dev_kfree_skb_any(skb) and returns NETDEV_TX_OK but leaves the queue stopped
via netif_stop_queue(dev).
Because no transmission is started, the hardware will never generate a
completion interrupt, leaving the queue permanently hung.
Additionally, ptr is left pointing to the freed memory, creating a dangling
pointer. Does this need to be cleared when the error occurs?
^ permalink raw reply
* Re: [PATCH net-next v6 6/7] net: macb: add XDP support for gem
From: Jakub Kicinski @ 2026-03-29 19:47 UTC (permalink / raw)
To: pvalerio
Cc: Jakub Kicinski, netdev, nicolas.ferre, claudiu.beznea,
andrew+netdev, davem, edumazet, pabeni, lorenzo, theo.lebrun, nb
In-Reply-To: <20260323221047.2749577-7-pvalerio@redhat.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: macb: add XDP support for gem
This commit introduces basic XDP support for macb/gem with the XDP_TX,
XDP_PASS, XDP_DROP, and XDP_REDIRECT verdict support.
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 4c5dd70f003b..537d02264851 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
> @@ -1197,6 +1198,18 @@ static int macb_halt_tx(struct macb *bp)
> bp, TSR);
> }
>
> +static void macb_tx_release_buff(void *buff, enum macb_tx_buff_type type, int budget)
> +{
> + if (type == MACB_TYPE_SKB) {
> + napi_consume_skb(buff, budget);
> + } else {
> + if (!budget)
> + xdp_return_frame(buff);
> + else
> + xdp_return_frame_rx_napi(buff);
> + }
> +}
Does this code cause memory corruption in the page pool lockless cache?
This function is called from macb_tx_complete(), which executes in the
context of the TX NAPI poll routine (queue->napi_tx).
However, the page pool was created and bound to the RX NAPI context
(queue->napi_rx). Since xdp_return_frame_rx_napi() bypasses safety checks
and places the page directly into the pool's lockless cache, and these are
independent NAPI instances that can be scheduled concurrently on different
CPUs, shouldn't this use xdp_return_frame() to safely handle cross-CPU
synchronization?
[ ... ]
> +static int macb_xdp_submit_frame(struct macb *bp, struct xdp_frame *xdpf,
> + struct net_device *dev, dma_addr_t addr)
> +{
[ ... ]
> + tx_buff->ptr = xdpf;
> + tx_buff->type = MACB_TYPE_XDP_TX;
> + tx_buff->mapping = 0;
> + tx_buff->size = xdpf->len;
> + tx_buff->mapped_as_page = false;
> +
> + ctrl = (u32)tx_buff->size;
> + ctrl |= MACB_BIT(TX_LAST);
Could this write overwrite control bits in the descriptor if the XDP frame
length exceeds the hardware's maximum limit?
The hardware uses a 14-bit field for the frame length, with higher bits
reserved for control flags like TX_LAST and TX_WRAP. On architectures with
a PAGE_SIZE larger than 16KB (e.g., ARM64 ZynqMP with 64KB pages), an XDP
program can use bpf_xdp_adjust_tail() to legitimately increase the frame
length beyond this boundary.
Does this need a bounds check against the hardware's maximum supported
transmit size to prevent corrupted DMA operations?
[ ... ]
> @@ -1683,6 +1878,18 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
> queue->stats.rx_dropped++;
> }
>
> + headroom = bp->rx_headroom;
> +
> + if (last_frame) {
> + ret = gem_xdp_run(queue, buff_head, &data_len,
> + &headroom, addr - gem_rx_pad(bp));
> + if (ret == XDP_REDIRECT)
> + xdp_flush = true;
> +
> + if (ret != XDP_PASS)
> + goto next_frame;
> + }
> +
> queue->skb = napi_build_skb(buff_head, gem_total_rx_buffer_size(bp));
Will this sequence allow multi-descriptor RX packets to silently bypass the
XDP program?
If a received packet spans multiple hardware descriptors (first_frame is true
but last_frame is false), the code skips gem_xdp_run() completely. It then
proceeds to allocate an sk_buff using napi_build_skb(), append subsequent
fragments on following loop iterations, and pass the assembled packet to the
network stack.
Shouldn't the driver explicitly drop packets that span multiple descriptors
when an XDP program is attached?
^ permalink raw reply
* Re: [PATCH net-next v6 7/7] net: macb: introduce ndo_xdp_xmit support
From: Jakub Kicinski @ 2026-03-29 19:47 UTC (permalink / raw)
To: pvalerio
Cc: Jakub Kicinski, netdev, nicolas.ferre, claudiu.beznea,
andrew+netdev, davem, edumazet, pabeni, lorenzo, theo.lebrun, nb
In-Reply-To: <20260323221047.2749577-8-pvalerio@redhat.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: macb: introduce ndo_xdp_xmit support
This commit introduces ndo_xdp_xmit support for the macb driver to
enable XDP redirection. It adds the gem_xdp_xmit function and updates
macb_xdp_submit_frame to handle DMA mapping for redirected frames.
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 537d02264851..38b15d1e7127 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
[ ... ]
> @@ -1708,8 +1723,8 @@ static int macb_xdp_submit_frame(struct macb *bp, struct xdp_frame *xdpf,
> desc = macb_tx_desc(queue, queue->tx_head);
> tx_buff = macb_tx_buff(queue, queue->tx_head);
> tx_buff->ptr = xdpf;
> - tx_buff->type = MACB_TYPE_XDP_TX;
> - tx_buff->mapping = 0;
> + tx_buff->type = buff_type;
> + tx_buff->mapping = dma_map ? mapping : 0;
> tx_buff->size = xdpf->len;
Is a bounds check needed on the frame length here?
Since ndo_xdp_xmit can receive frames redirected from other devices,
the frame length could potentially be up to ~64KB on architectures with
64KB pages.
macb_xdp_submit_frame() casts this length to a 32-bit integer and
bitwise-ORs it into the descriptor's control word, which has a 14-bit
length field (maximum 16383 bytes). Frame lengths exceeding this limit
could silently overflow into adjacent control bits like TX_LAST,
corrupting the descriptor ring.
[ ... ]
> +static int gem_xdp_xmit(struct net_device *dev, int num_frame,
> + struct xdp_frame **frames, u32 flags)
> +{
> + struct macb *bp = netdev_priv(dev);
> + u32 xmitted = 0;
> + int i;
> +
> + if (!macb_is_gem(bp))
> + return -EOPNOTSUPP;
> +
> + if (unlikely(!netif_carrier_ok(dev)))
> + return -ENETDOWN;
> +
> + if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> + return -EINVAL;
> +
> + for (i = 0; i < num_frame; i++) {
> + if (macb_xdp_submit_frame(bp, frames[i], dev, true, 0))
> + break;
Should this loop batch the doorbell rings based on the XDP_XMIT_FLUSH flag?
Currently, the code calls macb_xdp_submit_frame() for each frame, which
independently acquires the global bp->lock and performs an MMIO write to the
NCR register to ring the hardware doorbell per packet.
Doing this per packet ignores the batching semantics of the bulk transmission
API and could cause severe lock contention and MMIO overhead.
> +
> + xmitted++;
> + }
> +
> + return xmitted;
> +}
^ permalink raw reply
* Re: [PATCH net-next v6 6/7] net: macb: add XDP support for gem
From: Jakub Kicinski @ 2026-03-29 19:50 UTC (permalink / raw)
To: Paolo Valerio
Cc: netdev, Nicolas Ferre, Claudiu Beznea, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni, Lorenzo Bianconi,
Théo Lebrun, Nicolai Buchwitz
In-Reply-To: <20260323221047.2749577-7-pvalerio@redhat.com>
On Mon, 23 Mar 2026 23:10:46 +0100 Paolo Valerio wrote:
> + running = netif_running(dev);
> + need_update = !!bp->prog != !!prog;
> + if (running && need_update)
> + macb_close(dev);
> +
> + old_prog = rcu_replace_pointer(bp->prog, prog, lockdep_rtnl_is_held());
> +
> + if (running && need_update) {
> + err = macb_open(dev);
Full close + open is not allowed for new code.
^ permalink raw reply
* Re: [PATCH net-next v2] net: sfp: add quirk for ZOERAX SFP-2.5G-T
From: Russell King (Oracle) @ 2026-03-29 19:53 UTC (permalink / raw)
To: Jan Hoffmann
Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, linux-kernel
In-Reply-To: <20260329191304.720160-1-jan@3e8.eu>
On Sun, Mar 29, 2026 at 09:11:11PM +0200, Jan Hoffmann wrote:
> This is a 2.5G copper module which appears to be based on a Motorcomm
> YT8821 PHY. There doesn't seem to be a usable way to to access the PHY
> (I2C address 0x56 provides only read-only C22 access, and Rollball is
> also not working).
>
> The module does not report the correct extended compliance code for
> 2.5GBase-T, and instead claims to support SONET OC-48 and Fibre Channel:
>
> Identifier : 0x03 (SFP)
> Extended identifier : 0x04 (GBIC/SFP defined by 2-wire interface ID)
> Connector : 0x07 (LC)
> Transceiver codes : 0x00 0x01 0x00 0x00 0x40 0x40 0x04 0x00 0x00
> Transceiver type : FC: Multimode, 50um (M5)
> Encoding : 0x05 (SONET Scrambled)
> BR Nominal : 2500MBd
>
> Despite this, the kernel still enables the correct 2500Base-X interface
> mode. However, for the module to actually work, it is also necessary to
> disable inband auto-negotiation.
>
> Enable the existing "sfp_quirk_oem_2_5g" for this module, which handles
> that and also sets the bit for 2500Base-T link mode.
>
> Signed-off-by: Jan Hoffmann <jan@3e8.eu>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Thanks!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply
* Re: [PATCH net-next v2 1/9] net: dsa: add tag driver for LAN9645X
From: Jakub Kicinski @ 2026-03-29 19:56 UTC (permalink / raw)
To: jensemil.schulzostergaard
Cc: Jakub Kicinski, UNGLinuxDriver, andrew, olteanv, davem, edumazet,
pabeni, horms, robh, krzk+dt, conor+dt, woojung.huh, linux,
Steen.Hegelund, daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260324-dsa_lan9645x_switch_driver_base-v2-1-f7504e3b0681@microchip.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: dsa: add tag driver for LAN9645X
Add a tag driver for LAN9645x switches using a front port as the CPU port
(NPI port). The driver configures a long prefix for extraction and no
prefix for injection to handle the internal frame header (IFH).
> diff --git a/net/dsa/tag_lan9645x.c b/net/dsa/tag_lan9645x.c
> new file mode 100644
> index 000000000000..9130fb0d61be
> --- /dev/null
> +++ b/net/dsa/tag_lan9645x.c
[ ... ]
> +static void lan9645x_xmit_get_vlan_info(struct sk_buff *skb,
> + struct net_device *br,
> + u32 *vlan_tci, u32 *tag_type)
> +{
> + struct vlan_ethhdr *hdr;
> + u16 proto, tci;
> +
> + if (!br || !br_vlan_enabled(br)) {
> + *vlan_tci = 0;
> + *tag_type = LAN9645X_IFH_TAG_TYPE_C;
> + return;
> + }
> +
> + hdr = (struct vlan_ethhdr *)skb_mac_header(skb);
> + br_vlan_get_proto(br, &proto);
> +
> + if (ntohs(hdr->h_vlan_proto) == proto) {
> + vlan_remove_tag(skb, &tci);
Is it possible for a maliciously crafted packet (e.g., via AF_PACKET) to have a
length of 14 bytes with h_vlan_proto set to ETH_P_8021Q?
If so, vlan_remove_tag() would read vhdr->h_vlan_TCI at offset 14, causing an
out-of-bounds read.
Furthermore, if the packet contains fragments and the VLAN header is not in the
linear data area, vlan_remove_tag() could corrupt the SKB when shifting the MAC
header.
Would it be safer to use skb_vlan_untag() instead?
> + *vlan_tci = tci;
> + } else {
> + rcu_read_lock();
> + br_vlan_get_pvid_rcu(br, &tci);
> + rcu_read_unlock();
> + *vlan_tci = tci;
> + }
> +
> + *tag_type = (proto != ETH_P_8021Q) ? LAN9645X_IFH_TAG_TYPE_S :
> + LAN9645X_IFH_TAG_TYPE_C;
> +}
[ ... ]
> +static struct sk_buff *lan9645x_rcv(struct sk_buff *skb,
> + struct net_device *ndev)
> +{
> + u32 src_port, qos_class, vlan_tci, tag_type, popcnt, etype_ofs;
> + u8 *orig_skb_data = skb->data;
> + struct dsa_port *dp;
> + u32 ifh_gap_len = 0;
> + u16 vlan_tpid;
> + u8 *ifh;
> +
> + /* DSA master already consumed DMAC,SMAC,ETYPE from long prefix. Go back
> + * to beginning of frame.
> + */
> + skb_push(skb, ETH_HLEN);
> + /* IFH starts after our long prefix */
> + ifh = skb_pull(skb, LAN9645X_LONG_PREFIX_LEN);
Could skb_pull() return NULL here if skb->len is less than 16 bytes?
If it returns NULL, the blind dereference of ifh in the subsequent
lan9645x_ifh_get() calls would cause a NULL pointer dereference.
Additionally, is the 44-byte tag (16-byte prefix + 28-byte IFH) guaranteed to
be in the linear data buffer (skb->data)?
Without calling pskb_may_pull(skb, LAN9645X_TOTAL_TAG_LEN) first, accessing it
via the ifh pointer might result in out-of-bounds reads if the packet payload
is in paged fragments.
> +
> + popcnt = lan9645x_ifh_get(ifh, IFH_POP_CNT, IFH_POP_CNT_SZ);
> + etype_ofs = lan9645x_ifh_get(ifh, IFH_ETYPE_OFS, IFH_ETYPE_OFS_SZ);
> + src_port = lan9645x_ifh_get(ifh, IFH_SRCPORT, IFH_SRCPORT_SZ);
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH net-next v2 2/9] dt-bindings: net: lan9645x: add LAN9645X switch bindings
From: Jakub Kicinski @ 2026-03-29 19:56 UTC (permalink / raw)
To: jensemil.schulzostergaard
Cc: Jakub Kicinski, UNGLinuxDriver, andrew, olteanv, davem, edumazet,
pabeni, horms, robh, krzk+dt, conor+dt, woojung.huh, linux,
Steen.Hegelund, daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260324-dsa_lan9645x_switch_driver_base-v2-2-f7504e3b0681@microchip.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.
---
dt-bindings: net: lan9645x: add LAN9645X switch bindings
Add bindings for LAN9645X switch. We use a fallback compatible for the
smallest SKU microchip,lan96455s-switch.
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan96455s-switch.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan96455s-switch.yaml
> new file mode 100644
> index 000000000000..0282e25c05d4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan96455s-switch.yaml
[ ... ]
> +patternProperties:
> + "^(ethernet-)?ports$":
> + type: object
> + additionalProperties: true
> + patternProperties:
> + "^(ethernet-)?port@[0-8]$":
Does having additionalProperties set to true bypass the port number
restriction?
Since dsa.yaml allows port@[0-9a-f]+, it seems port@9 would silently pass
validation as an additional property. Should this be unevaluatedProperties
set to false instead?
> + type: object
> + description: Ethernet switch ports
> +
> + $ref: dsa-port.yaml#
> +
> + properties:
> + rx-internal-delay-ps:
> + const: 2000
> +
> + tx-internal-delay-ps:
> + const: 2000
Does enforcing a 2000ps delay conflict with standard rgmii bindings?
As documented in net/ethernet-controller.yaml, a full 2ns delay is
typically modeled using phy-mode set to rgmii-id rather than hardcoding the
delay properties with a base rgmii mode.
[ ... ]
> +required:
> + - compatible
> + - reg
> +
> +unevaluatedProperties: false
How are the five integrated 10/100/1000Base-T phys mentioned in the
description modeled?
Since unevaluatedProperties is false at the top level and there is no
definition or reference for an mdio node, wouldn't adding an mdio subnode
to configure these phys fail schema validation?
Should there be a reference to /schemas/net/mdio.yaml#?
^ permalink raw reply
* Re: [PATCH net-next v2 3/9] net: dsa: lan9645x: add autogenerated register macros
From: Jakub Kicinski @ 2026-03-29 19:56 UTC (permalink / raw)
To: jensemil.schulzostergaard
Cc: Jakub Kicinski, UNGLinuxDriver, andrew, olteanv, davem, edumazet,
pabeni, horms, robh, krzk+dt, conor+dt, woojung.huh, linux,
Steen.Hegelund, daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260324-dsa_lan9645x_switch_driver_base-v2-3-f7504e3b0681@microchip.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.
---
No issues found.
^ permalink raw reply
* Re: [PATCH net-next v2 4/9] net: dsa: lan9645x: add basic dsa driver for LAN9645X
From: Jakub Kicinski @ 2026-03-29 19:56 UTC (permalink / raw)
To: jensemil.schulzostergaard
Cc: Jakub Kicinski, UNGLinuxDriver, andrew, olteanv, davem, edumazet,
pabeni, horms, robh, krzk+dt, conor+dt, woojung.huh, linux,
Steen.Hegelund, daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260324-dsa_lan9645x_switch_driver_base-v2-4-f7504e3b0681@microchip.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.
---
Subject: net: dsa: lan9645x: add basic dsa driver for LAN9645X
This commit adds a basic DSA driver for the Microchip LAN9645X switch. It
includes initialization, port module setup for NPI, CPU, and front ports,
and phylink integration for MAC side configuration.
diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_npi.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_npi.c
--- a/drivers/net/dsa/microchip/lan9645x/lan9645x_npi.c
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_npi.c
[ ... ]
> +static void disable_conduit_ipv6(struct lan9645x *lan9645x,
> + struct net_device *conduit)
> +{
> + struct inet6_dev *dev_v6;
> +
> + if (!conduit)
> + return;
> +
> + /* IPv6 on the conduit will generate frames bypassing our tag driver, so
> + * they lack an IFH. This will be garbage in garbage out and we want to
> + * avoid this.
> + */
> + rtnl_lock();
> + dev_v6 = __in6_dev_get(conduit);
> + if (dev_v6) {
> + WRITE_ONCE(dev_v6->cnf.disable_ipv6, 1);
> + dev_warn(lan9645x->dev, "Disabled IPv6 on conduit device: %s\n",
> + netdev_name(conduit));
> + }
> + rtnl_lock();
> +}
Does directly modifying dev_v6->cnf.disable_ipv6 bypass the proper sysctl
teardown path (addrconf_disable_ipv6) and leave the IPv6 stack in an
inconsistent state on the conduit device by failing to clean up routes,
addresses, and MLD memberships?
Is it a layering violation for a switch driver to alter the networking
configuration of an external upper device, especially since this change
isn't reverted during driver unbind?
[ ... ]
> +void lan9645x_npi_port_init(struct lan9645x *lan9645x,
> + struct dsa_port *cpu_port)
> +{
> + int port = cpu_port->index;
> + struct lan9645x_port *p;
> +
> + p = lan9645x_to_port(lan9645x, port);
> + lan9645x->npi = port;
In a cascaded multi-switch DSA topology, cpu_port->index may belong to a
different switch and be greater than 8. If lan9645x_to_port returns NULL,
will this cause a panic when dereferencing p->chip_port below?
[ ... ]
> + /* Configure IFH prefix mode for NPI port. */
> + lan_rmw(SYS_PORT_MODE_INCL_XTR_HDR_SET(LAN9645X_TAG_PREFIX_LONG) |
> + SYS_PORT_MODE_INCL_INJ_HDR_SET(LAN9645X_TAG_PREFIX_NONE),
> + SYS_PORT_MODE_INCL_XTR_HDR |
> + SYS_PORT_MODE_INCL_INJ_HDR,
> + lan9645x,
> + SYS_PORT_MODE(p->chip_port));
Setting LAN9645X_TAG_PREFIX_NONE means the hardware interprets the first
bytes of any frame received from the host as an IFH command structure.
Does this allow a local user to transmit crafted frames (for example, via
raw sockets or standard protocols) whose MAC address bytes map to malicious
IFH commands, thereby bypassing VLAN isolation or flooding the switch?
Should the hardware be configured to require a strict prefix, such as
LAN9645X_TAG_PREFIX_LONG, for injected frames to prevent this?
diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
--- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> +static int lan9645x_setup(struct dsa_switch *ds)
> +{
> + struct lan9645x *lan9645x = ds->priv;
> + struct device *dev = lan9645x->dev;
> + u32 all_phys_ports;
> + int err;
> +
> + lan9645x->num_phys_ports = ds->num_ports;
> + all_phys_ports = GENMASK(lan9645x->num_phys_ports - 1, 0);
> +
> + err = lan9645x_reset_switch(lan9645x);
> + if (err)
> + return err;
> +
> + lan9645x->ports = devm_kcalloc(lan9645x->dev, lan9645x->num_phys_ports,
> + sizeof(struct lan9645x_port *),
> + GFP_KERNEL);
Since the DSA .setup callback can be invoked multiple times during the
platform device's lifetime (for example, when the DSA master interface is
unbound and bound again), does using devm_kcalloc here cause a memory leak?
Should these allocations be moved to .probe or managed with kcalloc and
freed in .teardown?
[ ... ]
> + /* Set all the entries to obey VLAN. */
> + for (int i = 0; i < PGID_ENTRIES; ++i)
> + lan_wr(ANA_PGID_CFG_OBEY_VLAN_SET(1),
> + lan9645x, ANA_PGID_CFG(i));
PGID_ENTRIES is defined as 89, so this loop initializes indices 0 through
88. Since the CPU port is index 9, its source PGID is PGID_SRC + CPU_PORT
(80 + 9 = 89).
Is index 89 left uninitialized, breaking the OBEY_VLAN rule and allowing
CPU-injected frames to leak across VLAN boundaries?
[ ... ]
> + /* Multicast to all front ports */
> + lan_wr(all_phys_ports, lan9645x, ANA_PGID(PGID_MC));
> +
> + /* IP multicast to all front ports */
> + lan_wr(all_phys_ports, lan9645x, ANA_PGID(PGID_MCIPV4));
> + lan_wr(all_phys_ports, lan9645x, ANA_PGID(PGID_MCIPV6));
> +
> + /* Unicast to all front ports */
> + lan_wr(all_phys_ports, lan9645x, ANA_PGID(PGID_UC));
> +
> + /* Broadcast to all ports */
> + lan_wr(BIT(CPU_PORT) | all_phys_ports, lan9645x, ANA_PGID(PGID_BC));
PGID_BC includes BIT(CPU_PORT) and all_phys_ports (which includes the NPI
port). Will this forward broadcast frames to both the CPU extraction queue
and the NPI port's normal egress queue, causing duplicate frames for the host?
Conversely, the multicast masks and PGID_UC exclude BIT(CPU_PORT). Does
this cause them to bypass the CPU extraction queue entirely, thereby
lacking the LONG extraction prefix and breaking the host's DSA tagger parsing?
diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
--- a/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_phylink.c
[ ... ]
> +static int lan9645x_phylink_mac_prepare(struct phylink_config *config,
> + unsigned int mode,
> + phy_interface_t iface)
> +{
> + struct lan9645x_port *p = lan9645x_phylink_config_to_port(config);
> + struct lan9645x *lan9645x = p->lan9645x;
> + int port = p->chip_port;
> + u32 mask;
> +
> + if (port == 5 || port == 6 || port > 8)
> + return -EINVAL;
> +
> + mask = HSIO_HW_CFG_GMII_ENA_SET(BIT(port));
> + lan_rmw(mask, mask, lan9645x, HSIO_HW_CFG);
> +
> + if (port == 4 && phy_interface_mode_is_rgmii(iface))
> + lan_rmw(HSIO_HW_CFG_RGMII_0_CFG_SET(1),
> + HSIO_HW_CFG_RGMII_0_CFG,
> + lan9645x, HSIO_HW_CFG);
The lan9645x_phylink_get_caps function advertises RGMII support for ports
4, 7, and 8. Does restricting the hardware multiplexer configuration here to
port 4 break connectivity for ports 7 and 8 when used in RGMII mode?
diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
--- a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
+++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
[ ... ]
> +int lan9645x_port_set_maxlen(struct lan9645x *lan9645x, int port, size_t sdu)
> +{
> + struct lan9645x_port *p = lan9645x_to_port(lan9645x, port);
> + int maxlen = sdu + ETH_HLEN + ETH_FCS_LEN;
Does this maximum frame length calculation drop standard 1500-byte MTU
frames that are 802.1Q VLAN tagged (1522 bytes total), since it does not
account for VLAN_HLEN?
[ ... ]
> +int lan9645x_port_setup(struct dsa_switch *ds, int port)
> +{
> + struct dsa_port *dp = dsa_to_port(ds, port);
> + struct lan9645x *lan9645x = ds->priv;
> + struct lan9645x_port *p;
> +
> + p = lan9645x_to_port(lan9645x, port);
> +
> + if (dp->dn) {
> + p->rx_internal_delay =
> + of_property_present(dp->dn, "rx-internal-delay-ps");
> + p->tx_internal_delay =
> + of_property_present(dp->dn, "tx-internal-delay-ps");
> + }
These are standard integer properties specifying delays in picoseconds. If
a user explicitly disables the delay via devicetree using a value of 0,
will of_property_present evaluate to true and enable the hardware delay
anyway? Should of_property_read_u32 be used instead to check the value?
^ permalink raw reply
* Re: [PATCH net-next v2 5/9] net: dsa: lan9645x: add bridge support
From: Jakub Kicinski @ 2026-03-29 19:56 UTC (permalink / raw)
To: jensemil.schulzostergaard
Cc: Jakub Kicinski, UNGLinuxDriver, andrew, olteanv, davem, edumazet,
pabeni, horms, robh, krzk+dt, conor+dt, woojung.huh, linux,
Steen.Hegelund, daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260324-dsa_lan9645x_switch_driver_base-v2-5-f7504e3b0681@microchip.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: dsa: lan9645x: add bridge support
This commit adds support for hardware offloading of the bridge in the
lan9645x DSA driver. It enables a single bridge device to be used with
the switch.
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> index a0908cbf89c7..599e589c4ec3 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> @@ -145,6 +155,8 @@ static int lan9645x_setup(struct dsa_switch *ds)
> return err;
> }
>
> + mutex_init(&lan9645x->fwd_domain_lock);
> +
> /* Link Aggregation Mode: NETDEV_LAG_HASH_L2 */
> lan_wr(ANA_AGGR_CFG_AC_SMAC_ENA |
> ANA_AGGR_CFG_AC_DMAC_ENA,
[ ... ]
> @@ -240,6 +252,11 @@ static int lan9645x_setup(struct dsa_switch *ds)
>
> lan9645x_port_set_tail_drop_wm(lan9645x);
>
> + lan9645x->owq = alloc_ordered_workqueue("%s-owq", 0,
> + dev_name(lan9645x->dev));
> + if (!lan9645x->owq)
> + return -ENOMEM;
> +
Is it possible that the hardware remains initialized if the call to
alloc_ordered_workqueue fails? Because the setup callback returns an error,
the DSA core will skip the teardown callback. This would bypass
lan9645x_npi_port_deinit, potentially leaving the conduit netdev in a
corrupted state with hardware tagging enabled.
Also in lan9645x_setup, the initialization loop for PGID_ENTRIES iterates
through index 88:
for (int i = 0; i < PGID_ENTRIES; ++i)
lan_wr(ANA_PGID_CFG_OBEY_VLAN_SET(1),
lan9645x, ANA_PGID_CFG(i));
Could this leave the CPU port's source configuration in an undefined state?
The CPU port is port 9, which has its source analysis register at PGID_SRC
(80) + 9 = 89, but the loop terminates before reaching it.
> @@ -258,6 +275,271 @@ static void lan9645x_port_phylink_get_caps(struct dsa_switch *ds, int port,
> lan9645x_phylink_get_caps(ds->priv, port, config);
> }
>
> +static int lan9645x_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
> +{
> + u32 age_secs = max(1, msecs / MSEC_PER_SEC / 2);
> + struct lan9645x *lan9645x = ds->priv;
> +
> + /* Entry is must suffer two aging scans before it is removed, so it is
> + * aged after 2*AGE_PERIOD, and the unit is in seconds.
> + * An age period of 0 disables automatic aging.
> + */
> + lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(age_secs),
> + ANA_AUTOAGE_AGE_PERIOD,
> + lan9645x, ANA_AUTOAGE);
> + return 0;
> +}
Does this function properly disable automatic aging when requested? When the
Linux bridge passes msecs = 0 to disable aging, the max macro evaluates to 1.
Since writing 1 configures an aggressive 2-second aging period, it appears
this might not fulfill the user's intent.
[ ... ]
> +static void __lan9645x_port_set_host_flood(struct lan9645x *lan9645x, int port,
> + bool uc, bool mc)
> +{
> + bool mc_ena, uc_ena;
> +
> + lockdep_assert_held(&lan9645x->fwd_domain_lock);
> +
> + /* We want promiscuous and all_multi to affect standalone ports, for
> + * debug and test purposes.
> + *
> + * However, the linux bridge is incredibly eager to put bridged ports in
> + * promiscuous mode.
> +
> + * This is unfortunate since lan9645x flood masks are global and not per
> + * ingress port. When some port triggers unknown uc/mc to the CPU, the
> + * traffic from any port is forwarded to the CPU.
> + *
> + * If the host CPU is weak, this can cause tremendous stress. Therefore,
> + * we compromise by ignoring this host flood request for bridged ports.
> + */
> + if (lan9645x_port_is_bridged(lan9645x_to_port(lan9645x, port)))
> + return;
Will ignoring the host flood request for bridged ports break IPv6 Neighbor
Discovery? IPv6 ND relies on multicast, and the PGID_MC and PGID_MCIPV6 masks
do not include the CPU port by default. By ignoring the bridge's request to
enable multicast flooding, the CPU port may fail to receive ICMPv6 ND packets.
[ ... ]
> +/* Called in atomic context */
> +static void lan9645x_port_set_host_flood(struct dsa_switch *ds, int port,
> + bool uc, bool mc)
> +{
> + struct lan9645x *lan9645x = ds->priv;
> + struct lan9645x_host_flood_work *w;
> +
> + w = kzalloc_obj(*w, GFP_ATOMIC);
Is kzalloc_obj an available kernel function? Using it might cause a build
error, as the standard API would be kzalloc(sizeof(*w), GFP_ATOMIC).
> + if (!w)
> + return;
Are there consequences to returning silently when the allocation fails?
If this allocation fails under memory pressure, the network stack will still
assume the configuration was successfully applied, leaving the hardware state
out of sync.
[ ... ]
> +static int lan9645x_port_bridge_join(struct dsa_switch *ds, int port,
> + struct dsa_bridge bridge,
> + bool *tx_fwd_offload,
> + struct netlink_ext_ack *extack)
> +{
[ ... ]
> + /* The bridge puts ports in IFF_ALLMULTI before calling
> + * port_bridge_join, so clean up before the port is marked as bridged.
> + */
> + __lan9645x_port_set_host_flood(lan9645x, port, false, false);
> + lan9645x->bridge_mask |= BIT(p->chip_port);
> +
> + mutex_unlock(&lan9645x->fwd_domain_lock);
> +
> + /* Later: stp_state_set updates forwarding */
> +
> + return 0;
> +}
Could clearing the host flood masks here lead to lost hardware state when the
port leaves the bridge? If a user previously enabled IFF_PROMISC or
IFF_ALLMULTI on the standalone port, the software netdev flags remain set.
When lan9645x_port_bridge_leave is called, it does not restore the hardware's
promiscuous or multicast state to match the unchanged software flags.
> +static void lan9645x_port_bridge_stp_state_set(struct dsa_switch *ds, int port,
> + u8 state)
> +{
> + struct lan9645x *lan9645x;
> + struct lan9645x_port *p;
> + bool learn_ena;
> +
> + lan9645x = ds->priv;
> + p = lan9645x_to_port(lan9645x, port);
> +
> + mutex_lock(&lan9645x->fwd_domain_lock);
> +
> + p->stp_state = state;
> +
> + if (state == BR_STATE_FORWARDING)
> + lan9645x->bridge_fwd_mask |= BIT(p->chip_port);
> + else
> + lan9645x->bridge_fwd_mask &= ~BIT(p->chip_port);
> +
> + learn_ena = (state == BR_STATE_LEARNING ||
> + state == BR_STATE_FORWARDING) && p->learn_ena;
> +
> + lan_rmw(ANA_PORT_CFG_LEARN_ENA_SET(learn_ena),
> + ANA_PORT_CFG_LEARN_ENA, lan9645x,
> + ANA_PORT_CFG(p->chip_port));
> +
> + lan9645x_update_fwd_mask(lan9645x);
> + mutex_unlock(&lan9645x->fwd_domain_lock);
> +}
Are ingress data frames correctly dropped for ports in non-forwarding STP
states? When the port is removed from bridge_fwd_mask,
lan9645x_update_fwd_mask calculates the source PGID mask as
bridge_mask & bridge_fwd_mask & ~BIT(dp->index).
This evaluates to the set of all other forwarding ports in the bridge. Since
ingress forwarding is not explicitly disabled on the port (for example, by
setting ANA_PORT_CFG_PORTID_VAL to NONE), data frames received on a blocking
or learning port can enter the analyzer and be forwarded.
> +static void lan9645x_port_bridge_leave(struct dsa_switch *ds, int port,
> + struct dsa_bridge bridge)
> +{
> + struct lan9645x *lan9645x = ds->priv;
> + struct lan9645x_port *p;
> +
> + p = lan9645x_to_port(lan9645x, port);
> +
> + mutex_lock(&lan9645x->fwd_domain_lock);
> +
> + lan9645x->bridge_mask &= ~BIT(p->chip_port);
> +
> + /* Last port leaving clears bridge dev */
> + if (!lan9645x->bridge_mask)
> + lan9645x->bridge = NULL;
> +
> + lan9645x_update_fwd_mask(lan9645x);
> +
> + mutex_unlock(&lan9645x->fwd_domain_lock);
> +}
Should p->learn_ena be reset when a port leaves the bridge?
During port_bridge_flags, p->learn_ena is set to true. When leaving the
bridge, this flag remains true. The DSA core will transition the leaving
port to BR_STATE_FORWARDING, which calls lan9645x_port_bridge_stp_state_set
and leaves hardware learning enabled. This can pollute the shared FDB with
MAC addresses from standalone ports, leading to silent packet drops if
bridged ports attempt to forward traffic to them.
^ permalink raw reply
* Re: [PATCH net-next v2 6/9] net: dsa: lan9645x: add vlan support
From: Jakub Kicinski @ 2026-03-29 19:56 UTC (permalink / raw)
To: jensemil.schulzostergaard
Cc: Jakub Kicinski, UNGLinuxDriver, andrew, olteanv, davem, edumazet,
pabeni, horms, robh, krzk+dt, conor+dt, woojung.huh, linux,
Steen.Hegelund, daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260324-dsa_lan9645x_switch_driver_base-v2-6-f7504e3b0681@microchip.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: dsa: lan9645x: add vlan support
Add support for vlanaware bridge. We reserve vid 4095 for standalone
mode, to implement fdb-isolation. A vlan-unaware bridge uses vid 0.
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> index 599e589c4ec3..046c95a72242 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> @@ -535,11 +536,54 @@ static void lan9645x_port_bridge_leave(struct dsa_switch *ds, int port,
[ ... ]
> +static int lan9645x_port_vlan_filtering(struct dsa_switch *ds, int port,
> + bool enabled,
> + struct netlink_ext_ack *extack)
> +{
> + struct lan9645x *lan9645x = ds->priv;
> + struct lan9645x_port *p;
> +
> + p = lan9645x_to_port(lan9645x, port);
> + p->vlan_aware = enabled;
> + lan9645x_vlan_port_apply(p);
In other places, the driver avoids calling egress functions on the npi
port because it conflicts with cpu port header extraction. Since this
unconditionally applies the vlan configuration to both ingress and egress,
could this overwrite the npi port's configuration when the state is
synchronized?
> +
> + return 0;
> +}
> +
> +static int lan9645x_port_vlan_add(struct dsa_switch *ds, int port,
[ ... ]
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c
> new file mode 100644
> index 000000000000..0459932a626c
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_vlan.c
[ ... ]
> +static void lan9645x_vlan_hw_wr(struct lan9645x *lan9645x, u16 vid)
> +{
> + struct lan9645x_vlan *v = &lan9645x->vlans[vid];
> + bool cpu_dis = !(v->portmask & BIT(CPU_PORT));
> + u32 val;
> +
> + val = ANA_VLANTIDX_VLAN_PGID_CPU_DIS_SET(cpu_dis) |
> + ANA_VLANTIDX_V_INDEX_SET(vid) |
> + ANA_VLANTIDX_VLAN_SEC_FWD_ENA_SET(v->s_fwd_ena) |
> + ANA_VLANTIDX_VLAN_FLOOD_DIS_SET(v->fld_dis) |
> + ANA_VLANTIDX_VLAN_PRIV_VLAN_SET(v->prv_vlan) |
> + ANA_VLANTIDX_VLAN_LEARN_DISABLED_SET(v->lrn_dis) |
> + ANA_VLANTIDX_VLAN_MIRROR_SET(v->mir) |
> + ANA_VLANTIDX_VLAN_SRC_CHK_SET(v->src_chk);
> +
> + lan_wr(val, lan9645x, ANA_VLANTIDX);
> + lan_wr(v->portmask, lan9645x, ANA_VLAN_PORT_MASK);
> + lan_wr(VLANACCESS_CMD_WRITE, lan9645x, ANA_VLANACCESS);
Are these registers written with raw values rather than using their respective
set macros? This circumvents the register abstraction and might risk failures
if the hardware layout changes.
> +
> + if (lan9645x_vlan_wait_for_completion(lan9645x))
> + dev_err(lan9645x->dev, "Vlan set mask failed\n");
> +}
[ ... ]
> +static void
> +lan9645x_vlan_port_apply_egress(struct lan9645x_port *p,
> + struct lan9645x_vlan_port_info *info)
> +{
[ ... ]
> + lan_rmw(REW_TAG_CFG_TAG_TPID_CFG_SET(3) |
> + REW_TAG_CFG_TAG_CFG_SET(tag_cfg),
> + REW_TAG_CFG_TAG_TPID_CFG |
> + REW_TAG_CFG_TAG_CFG,
> + lan9645x, REW_TAG_CFG(p->chip_port));
> +
> + lan_rmw(REW_PORT_VLAN_CFG_PORT_TPID_SET(ETH_P_8021AD) |
> + REW_PORT_VLAN_CFG_PORT_VID_SET(port_vid),
> + REW_PORT_VLAN_CFG_PORT_TPID |
> + REW_PORT_VLAN_CFG_PORT_VID,
> + lan9645x, REW_PORT_VLAN_CFG(p->chip_port));
> +}
Setting the port tpid to 0x88a8 causes untagged ingress frames to egress trunk
ports with 802.1ad tags. Could this break interoperability with standard
networks that expect 0x8100 tags?
> +static void lan9645x_vlan_port_apply_ingress(struct lan9645x_port *p)
> +{
[ ... ]
> + /* Drop frames with multicast source address */
> + val = ANA_DROP_CFG_DROP_MC_SMAC_ENA_SET(1);
> + if (p->vlan_aware && !pvid)
> + /* If port is vlan-aware and tagged, drop untagged and priority
> + * tagged frames.
> + */
> + val |= ANA_DROP_CFG_DROP_UNTAGGED_ENA_SET(1) |
> + ANA_DROP_CFG_DROP_PRIO_S_TAGGED_ENA_SET(1) |
> + ANA_DROP_CFG_DROP_PRIO_C_TAGGED_ENA_SET(1);
> +
> + lan_wr(val, lan9645x, ANA_DROP_CFG(p->chip_port));
> +}
Does this overwrite the entire 32-bit register and silently clear other
initialized bitfields? Would it be safer to use a read-modify-write here
instead?
[ ... ]
> +static int lan9645x_vlan_cpu_add(struct lan9645x_port *p, u16 vid, bool pvid,
> + bool untagged)
> +{
> + struct lan9645x_vlan *v;
> +
> + v = lan9645x_vlan_port_modify(p, vid, pvid, untagged);
Is it intentional that this bypasses the bounds check on the vlan id?
> + v->portmask |= BIT(CPU_PORT) | BIT(p->chip_port);
> + lan9645x_vlan_hw_wr(p->lan9645x, vid);
> + lan9645x_vlan_port_apply_ingress(p);
> +
> + return 0;
> +}
> +
> +int lan9645x_vlan_port_add_vlan(struct lan9645x_port *p, u16 vid, bool pvid,
> + bool untagged, struct netlink_ext_ack *extack)
> +{
[ ... ]
> +void lan9645x_vlan_port_del_vlan(struct lan9645x_port *p, u16 vid)
> +{
> + struct lan9645x *lan9645x = p->lan9645x;
> + struct lan9645x_vlan *v;
> +
> + if (!vid)
> + return;
> +
> + v = lan9645x_vlan_port_modify(p, vid, false, false);
There is a check for the maximum vlan id when adding a vlan, but no bounds
check when deleting one. Could userspace use this to delete the reserved
vid 4095 and break standalone forwarding?
> + v->portmask &= ~BIT(p->chip_port);
> +
> + if (p->chip_port == lan9645x->npi)
> + v->portmask &= ~BIT(CPU_PORT);
> +
> + lan9645x_vlan_hw_wr(lan9645x, vid);
[ ... ]
> +void lan9645x_vlan_init(struct lan9645x *lan9645x)
> +{
[ ... ]
> + for (vid = 1; vid < VLAN_N_VID; vid++)
> + lan9645x_vlan_hw_wr(lan9645x, vid);
If the indirect table access times out, the write function prints an error
but does not return a status code. Will this cause the initialization loop
to stall for the full timeout duration 4095 consecutive times if the
hardware is unresponsive?
> +
> + /* Set all the ports + cpu to be part of HOST_PVID and UNAWARE_PVID */
> + lan9645x->vlans[HOST_PVID].portmask = all_ports;
^ permalink raw reply
* Re: [PATCH net-next v2 7/9] net: dsa: lan9645x: add mac table integration
From: Jakub Kicinski @ 2026-03-29 19:56 UTC (permalink / raw)
To: jensemil.schulzostergaard
Cc: Jakub Kicinski, UNGLinuxDriver, andrew, olteanv, davem, edumazet,
pabeni, horms, robh, krzk+dt, conor+dt, woojung.huh, linux,
Steen.Hegelund, daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260324-dsa_lan9645x_switch_driver_base-v2-7-f7504e3b0681@microchip.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: dsa: lan9645x: add mac table integration
Add MAC table support, and dsa fdb callback integration. The mactable is
keyed on (vid,mac) and each bucket has 4 slots. A mac table entry typically
points to a PGID index, the first 9 of which represent a front port.
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c
> index 000000000000..6335714dca21
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c
[ ... ]
> +int lan9645x_mact_entry_add(struct lan9645x *lan9645x, int pgid,
> + const unsigned char *mac, u16 vid)
> +{
> + struct lan9645x_mact_entry *entry;
> + int err;
> +
> + mutex_lock(&lan9645x->mact_lock);
> +
> + /* Users can not move (vid,mac) to a different port, without removing
> + * the original entry first. But we overwrite entry in HW, and update
> + * software pgid for good measure.
> + */
> + entry = lan9645x_mact_entry_find(lan9645x, mac, vid);
> + if (entry) {
> + entry->common.pgid = pgid;
> + goto mac_learn;
> + }
> +
> + entry = lan9645x_mact_entry_alloc(lan9645x, mac, vid, pgid,
> + ENTRYTYPE_LOCKED);
> + if (!entry) {
> + mutex_unlock(&lan9645x->mact_lock);
> + return -ENOMEM;
> + }
> +
> + list_add_tail(&entry->list, &lan9645x->mac_entries);
> +
> +mac_learn:
> + err = __lan9645x_mact_learn(lan9645x, pgid, mac, vid, ENTRYTYPE_LOCKED);
> + if (err)
> + lan9645x_mact_entry_dealloc(lan9645x, entry);
If a pre-existing entry is updated but the hardware learning command fails,
will this inadvertently delete the valid software entry and cause the hardware
and software tracking to become desynchronized?
> +
> + mutex_unlock(&lan9645x->mact_lock);
> + return err;
> +}
> +
> +int lan9645x_mact_entry_del(struct lan9645x *lan9645x, int pgid,
> + const unsigned char *mac, u16 vid)
> +{
> + struct lan9645x_mact_entry *entry;
> + int err;
> +
> + mutex_lock(&lan9645x->mact_lock);
> +
> + entry = lan9645x_mact_entry_find(lan9645x, mac, vid);
> + if (!entry) {
> + mutex_unlock(&lan9645x->mact_lock);
> + return -ENOENT;
> + }
> +
> + WARN_ON(entry->common.pgid != pgid);
> + lan9645x_mact_entry_dealloc(lan9645x, entry);
> + err = __lan9645x_mact_forget(lan9645x, mac, vid, ENTRYTYPE_LOCKED);
Does this permanently remove the software entry even if the hardware
command to forget the entry fails?
> +
> + mutex_unlock(&lan9645x->mact_lock);
> + return err;
> +}
[ ... ]
> +void lan9645x_mac_deinit(struct lan9645x *lan9645x)
> +{
> + mutex_destroy(&lan9645x->mact_lock);
> +}
Are the static MAC entries in the mac_entries list leaked here during driver
teardown, or are they freed elsewhere?
> +
> +int lan9645x_mact_dsa_dump(struct lan9645x *lan9645x, int port,
> + dsa_fdb_dump_cb_t *cb, void *data)
> +{
> + struct lan9645x_mact_entry entry = {};
> + u32 mach, macl, maca;
> + int err = 0;
> + u32 autoage;
[ ... ]
> + while (1) {
> + /* NOTE: we rely on mach, macl and type being set correctly in
> + * the registers from previous round, vis a vis the GET_NEXT
> + * semantics, so locking entire loop is important.
> + */
> + lan_wr(ANA_MACACCESS_MAC_TABLE_CMD_SET(CMD_GET_NEXT) |
> + ANA_MACACCESS_ENTRYTYPE_SET(entry.common.type),
> + lan9645x, ANA_MACACCESS);
> +
> + if (lan9645x_mac_wait_for_completion(lan9645x, &maca))
> + break;
If the hardware completion times out, the loop breaks out and returns the
local err variable which is initialized to 0. Will this suppress the hardware
timeout error code and incorrectly report a successful dump?
> +
> + if (ANA_MACACCESS_VALID_GET(maca) == 0)
> + break;
> +
> + mach = lan_rd(lan9645x, ANA_MACHDATA);
> + macl = lan_rd(lan9645x, ANA_MACLDATA);
> +
> + lan9645x_mact_parse(mach, macl, maca, &entry.common);
> +
> + if (ANA_MACACCESS_DEST_IDX_GET(maca) == port &&
> + entry.common.type == ENTRYTYPE_NORMAL) {
Since static FDB entries are programmed with ENTRYTYPE_LOCKED, will this
condition exclude them from the FDB dump, preventing them from showing up
during bridge FDB queries?
> + if (entry.common.key.vid > VLAN_MAX)
> + entry.common.key.vid = 0;
> +
> + err = cb(entry.common.key.mac, entry.common.key.vid,
> + false, data);
> + if (err)
> + break;
> + }
> + }
> +
> + /* Remove aging filters and restore aging */
> + lan_wr(0, lan9645x, ANA_ANAGEFIL);
> + lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(ANA_AUTOAGE_AGE_PERIOD_GET(autoage)),
> + ANA_AUTOAGE_AGE_PERIOD,
> + lan9645x, ANA_AUTOAGE);
Does restoring the ANA_AUTOAGE register using the autoage value saved at the
start of the function race with other operations setting the aging time?
If the aging time is updated concurrently without taking mact_lock, its newly
configured aging period might be overwritten here.
> +
> + mutex_unlock(&lan9645x->mact_lock);
> +
> + return err;
> +}
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> index 046c95a72242..32c0301030a4 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> +static int lan9645x_fdb_del(struct dsa_switch *ds, int port,
> + const unsigned char *addr, u16 vid,
> + struct dsa_db db)
> +{
> + struct net_device *br = lan9645x_db2bridge(db);
> + struct dsa_port *dp = dsa_to_port(ds, port);
> + struct lan9645x *lan9645x = ds->priv;
> + int err;
> +
> + if (IS_ERR(br))
> + return PTR_ERR(br);
> +
> + if (dsa_port_is_cpu(dp) && !br &&
> + dsa_fdb_present_in_other_db(ds, port, addr, vid, db))
> + return 0;
> +
> + if (!vid)
> + vid = lan9645x_vlan_unaware_pvid(!!br);
> +
> + if (dsa_port_is_cpu(dp))
> + return lan9645x_mact_forget(lan9645x, addr, vid,
> + ENTRYTYPE_LOCKED);
> +
> + err = lan9645x_mact_entry_del(lan9645x, port, addr, vid);
> + if (err == -ENOENT) {
> + dev_dbg(lan9645x->dev,
> + "fdb not found port=%d addr=%pM vid=%u\n", port, addr,
> + vid);
> + return 0;
> + }
> +
> + return err;
> +}
Since lan9645x_mact_entry_del returns -ENOENT when a MAC address is not found
in the software tracking list, and the software list only contains statically
added entries, won't this intercept attempts to manually delete dynamically
learned MAC addresses and return success without actually removing them from
the hardware table?
^ permalink raw reply
* Re: [PATCH net-next v2 8/9] net: dsa: lan9645x: add mdb management
From: Jakub Kicinski @ 2026-03-29 19:56 UTC (permalink / raw)
To: jensemil.schulzostergaard
Cc: Jakub Kicinski, UNGLinuxDriver, andrew, olteanv, davem, edumazet,
pabeni, horms, robh, krzk+dt, conor+dt, woojung.huh, linux,
Steen.Hegelund, daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260324-dsa_lan9645x_switch_driver_base-v2-8-f7504e3b0681@microchip.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.
---
Subject: net: dsa: lan9645x: add mdb management
Add support for dsa mdb callbacks. L2 multicast and IP multicast is
handled differently. IP multicast stores the port group mask inline in
the mac table. L2 multicast points to a PGID index, which encodes the
port group mask.
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> new file mode 100644
> index 000000000000..4cf92887a4eb
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> @@ -0,0 +1,391 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2026 Microchip Technology Inc.
> + */
> +
> +#include "lan9645x_main.h"
[ ... ]
> +static void lan9645x_mdb_encode_mac(unsigned char *mac,
> + struct lan9645x_mdb_entry *mdb_entry,
> + enum macaccess_entry_type type)
> +{
> + ether_addr_copy(mac, mdb_entry->mac);
> +
> + /* The HW encodes the portmask in the high bits of the mac for ip
> + * multicast entries, to save on the limited PGID resources.
> + *
> + * IPv4 Multicast DMAC: 0x01005Exxxxxx
> + * IPv6 Multicast DMAC: 0x3333xxxxxxxx
> + *
> + * which gives us 24 or 16 bits to encode the portmask.
> + */
> + if (type == ENTRYTYPE_MACV4) {
> + mac[0] = 0;
> + mac[1] = mdb_entry->ports >> 8;
> + mac[2] = mdb_entry->ports & 0xff;
> + } else if (type == ENTRYTYPE_MACV6) {
> + mac[0] = mdb_entry->ports >> 8;
> + mac[1] = mdb_entry->ports & 0xff;
> + }
> +}
Does this code need to mask out BIT(CPU_PORT) from mdb_entry->ports?
For L2 multicast, the CPU port is explicitly stripped from the PGID portmask
to avoid conflicting instructions, since CPU_PORT forwarding is handled by the
cpu_copy flag. Here, the IP multicast portmask is encoded directly into the
MAC address using the full mdb_entry->ports. Could this cause hardware to
receive conflicting instructions (forwarding to port 9 and copying to the CPU
extraction queue), leading to duplicate multicast packets for the CPU?
[ ... ]
> +static struct lan9645x_pgid_entry *
> +lan9645x_mdb_pgid_entry_create(struct lan9645x *lan9645x, u16 ports)
> +{
> + struct lan9645x_pgid_entry *pgid_entry = NULL;
> + int index;
> +
> + for (index = PGID_GP_START; index < PGID_GP_END; index++) {
Is it safe to start the search at PGID_GP_START (defined as CPU_PORT)?
Hardware uses ANA_PGID(DEST_IDX) to forward unicast traffic, and MAC table
entries for the CPU have DEST_IDX = 9. When the first L2 multicast group is
added, it appears this loop will allocate PGID index 9, overwriting the
unicast portmask for the CPU port. Could this cause all unicast traffic
destined for the CPU to be flooded to the external physical ports in the
multicast group instead?
> + bool used = false;
> +
> + list_for_each_entry(pgid_entry, &lan9645x->pgid_entries, list) {
> + if (pgid_entry->index == index) {
> + used = true;
> + break;
> + }
> + }
> +
> + if (!used)
> + return lan9645x_pgid_entry_alloc(lan9645x, index,
> + ports);
> + }
> +
> + return ERR_PTR(-ENOSPC);
> +}
[ ... ]
> +static int __lan9645x_mdb_add(struct lan9645x *lan9645x, int chip_port,
> + const unsigned char addr[ETH_ALEN], u16 vid,
> + enum macaccess_entry_type type)
> +{
> + struct lan9645x_pgid_entry *old_pgid, *new_pgid;
> + struct lan9645x_mdb_entry *mdb_entry;
> + unsigned char mac[ETH_ALEN];
> + int err, pgid_index;
> + bool cpu_copy;
> +
> + mdb_entry = lan9645x_mdb_entry_lookup(lan9645x, addr, vid);
> + if (!mdb_entry) {
> + mdb_entry = lan9645x_mdb_entry_alloc(lan9645x, addr, vid);
> + if (IS_ERR(mdb_entry))
> + return PTR_ERR(mdb_entry);
> + }
> +
> + if (mdb_entry->ports & BIT(chip_port))
> + return 0;
> +
> + mdb_entry->ports |= BIT(chip_port);
> +
> + /* Encode mac for IP mc */
> + lan9645x_mdb_encode_mac(mac, mdb_entry, type);
> +
> + /* Update PGID ptr for non-IP entries (L2 multicast) */
> + old_pgid = mdb_entry->pgid;
> + new_pgid = lan9645x_mdb_pgid_entry_get(lan9645x, mdb_entry, type);
> + if (IS_ERR(new_pgid)) {
> + /* Out of PGIDs or mem. Continue forwarding to old port
> + * group, or remove if fresh mdb_entry.
> + */
> + mdb_entry->ports &= ~BIT(chip_port);
> + if (!mdb_entry->ports)
> + lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> +
> + return PTR_ERR(new_pgid);
> + }
> + mdb_entry->pgid = new_pgid;
> +
> + cpu_copy = !!(mdb_entry->ports & BIT(CPU_PORT));
> + pgid_index = lan9645x_mdb_pgid_index(mdb_entry, type);
> +
> + /* Make sure to write on top of existing entry, so we do not disrupt
> + * flowing traffic.
> + */
> + err = lan9645x_mact_learn_cpu_copy(lan9645x, pgid_index, mac,
> + mdb_entry->vid, type, cpu_copy);
> + lan9645x_pgid_entry_put(lan9645x, old_pgid);
> + return err;
> +}
What happens if lan9645x_mact_learn_cpu_copy() fails and returns an error?
The software MDB state (mdb_entry->ports and mdb_entry->pgid) is updated and
the old PGID reference is dropped via lan9645x_pgid_entry_put() before
verifying if the hardware MAC table update succeeds.
If the hardware update fails, the software state isn't rolled back, leaving
them out of sync. Furthermore, the dropped old_pgid reference could allow the
index to be reallocated to a new multicast group while the hardware still
points to the old PGID index, potentially causing silent traffic cross-talk.
> +
> +static int __lan9645x_mdb_del(struct lan9645x *lan9645x, int chip_port,
> + const unsigned char addr[ETH_ALEN], u16 vid,
> + enum macaccess_entry_type type)
> +{
> + struct lan9645x_pgid_entry *old_pgid, *new_pgid;
> + struct lan9645x_mdb_entry *mdb_entry;
> + unsigned char mac[ETH_ALEN];
> + int err, pgid_index;
> + bool cpu_copy;
> +
> + mdb_entry = lan9645x_mdb_entry_lookup(lan9645x, addr, vid);
> + if (!mdb_entry)
> + return -ENOENT;
> +
> + if (!(mdb_entry->ports & BIT(chip_port)))
> + return 0;
> +
> + mdb_entry->ports &= ~BIT(chip_port);
> +
> + /* Encode mac for IP mc */
> + lan9645x_mdb_encode_mac(mac, mdb_entry, type);
> +
> + /* Update PGID ptr for non-IP entries (L2 multicast) */
> + old_pgid = mdb_entry->pgid;
> + new_pgid = lan9645x_mdb_pgid_entry_get(lan9645x, mdb_entry, type);
> + if (IS_ERR(new_pgid)) {
> + /* Continue forwarding to old port group. */
> + mdb_entry->ports |= BIT(chip_port);
> + return PTR_ERR(new_pgid);
> + }
> + mdb_entry->pgid = new_pgid;
> +
> + if (!mdb_entry->ports) {
> + lan9645x_mact_forget(lan9645x, mac, mdb_entry->vid, type);
> + lan9645x_pgid_entry_put(lan9645x, old_pgid);
> + lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> + return 0;
> + }
Similar to the addition path above, if lan9645x_mact_forget() fails, its
return value is ignored, and the software MDB entry and PGID are freed
anyway. This could also lead to hardware/software state desynchronization.
> +
> + cpu_copy = !!(mdb_entry->ports & BIT(CPU_PORT));
> + pgid_index = lan9645x_mdb_pgid_index(mdb_entry, type);
> +
> + err = lan9645x_mact_learn_cpu_copy(lan9645x, pgid_index, mac,
> + mdb_entry->vid, type, cpu_copy);
> + lan9645x_pgid_entry_put(lan9645x, old_pgid);
> + return err;
> +}
^ permalink raw reply
* Re: [PATCH net-next v2 9/9] net: dsa: lan9645x: add port statistics
From: Jakub Kicinski @ 2026-03-29 19:56 UTC (permalink / raw)
To: jensemil.schulzostergaard
Cc: Jakub Kicinski, UNGLinuxDriver, andrew, olteanv, davem, edumazet,
pabeni, horms, robh, krzk+dt, conor+dt, woojung.huh, linux,
Steen.Hegelund, daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260324-dsa_lan9645x_switch_driver_base-v2-9-f7504e3b0681@microchip.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: dsa: lan9645x: add port statistics
Add statistics support for the port counters. Chip registers are 32 bit,
so this unit is responsible maintaining a 64bit software cache, and
updating it frequently to handle overflows in hardware.
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> index 24a60f40f6b8..9a46c08dad59 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
[ ... ]
> @@ -262,6 +264,12 @@ static int lan9645x_setup(struct dsa_switch *ds)
> if (!lan9645x->owq)
> return -ENOMEM;
>
> + err = lan9645x_stats_init(lan9645x);
> + if (err) {
> + dev_err(dev, "Failed to init stats.\n");
> + goto owq_destroy;
> + }
> +
[ ... ]
> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
> index 000000000000..3e82a859f173
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
[ ... ]
> +static const struct lan9645x_ethtool_stat lan9645x_port_ethtool_stats[] = {
> + { "rx_uc", SCNT_RX_UC },
> + { "rx_cat_drop", SCNT_RX_CAT_DROP },
[ ... ]
> + { "tx_uc", SCNT_TX_UC },
> + { "tx_drop", SCNT_TX_DROP },
[ ... ]
> +};
Is it necessary to duplicate standard networking statistics in the driver
private ethtool output?
The networking subsystem guidelines state that stats possessing a standard UAPI
representation, such as tx_dropped and rx_packets in struct rtnl_link_stats64,
should not be duplicated in the ethtool -S output.
Could these redundant strings be removed since the driver natively supports
them via lan9645x_stats_get_stats64?
[ ... ]
> +static const struct lan9645x_view_stats lan9645x_view_stat_cfgs[] = {
> + [LAN9645X_STAT_PORTS] = {
> + .name = "ports",
> + .type = LAN9645X_STAT_PORTS,
> + .layout = lan9645x_port_stats_layout,
> + .num_cnts = ARRAY_SIZE(lan9645x_port_stats_layout),
> + .num_indexes = NUM_PHYS_PORTS,
> + },
> +};
[ ... ]
> +static u64 *lan9645x_stats_view_idx_update(struct lan9645x *lan9645x,
> + enum lan9645x_view_stat_type vtype,
> + int idx)
> +{
> + struct lan9645x_stats *s = lan9645x->stats;
> +
> + mutex_lock(&s->hw_lock);
> + __lan9645x_stats_view_idx_update(lan9645x, vtype, idx);
> + mutex_unlock(&s->hw_lock);
> +
> + return STAT_COUNTERS(lan9645x, vtype, idx);
> +}
Does this code overflow the allocated buffer when called for the CPU port?
The stats buffer cnts is allocated using num_indexes, which is NUM_PHYS_PORTS
(9). This covers ports 0 to 8. However, the CPU port is defined as index 9
(CPU_PORT = 9).
When the DSA core invokes stat callbacks like get_ethtool_stats or get_stats64
for the CPU port, the idx argument is 9. While the helper
__lan9645x_stats_view_idx_update skips the hardware read for out-of-bounds
indices, lan9645x_stats_view_idx_update ignores that failure and returns the
result of STAT_COUNTERS(lan9645x, vtype, idx).
This computes a pointer past the end of the cnts array, which the callers then
blindly dereference, potentially leaking kernel memory or causing a crash. Can
bounds checking be added to the callers, or should the buffer be sized to
include the CPU port?
[ ... ]
> +void lan9645x_stats_get_ethtool_stats(struct lan9645x *lan9645x, int port,
> + u64 *data)
> +{
> + struct lan9645x_stats *stats = lan9645x->stats;
> + u64 *c;
> + int i;
> +
> + c = lan9645x_stats_view_idx_update(lan9645x, LAN9645X_STAT_PORTS, port);
> +
> + spin_lock(&stats->sw_lock);
> + for (i = 0; i < ARRAY_SIZE(lan9645x_port_ethtool_stats); i++)
> + *data++ = c[lan9645x_port_ethtool_stats[i].idx];
> + spin_unlock(&stats->sw_lock);
> +}
[ ... ]
> +/* Called in atomic context */
> +void lan9645x_stats_get_stats64(struct lan9645x *lan9645x, int port,
> + struct rtnl_link_stats64 *stats)
> +{
> + struct lan9645x_stats *s = lan9645x->stats;
> + u64 *c;
> +
> + c = STAT_COUNTERS(lan9645x, LAN9645X_STAT_PORTS, port);
> +
> + spin_lock(&s->sw_lock);
Can this code deadlock when a softirq interrupts a process holding the lock?
The sw_lock spinlock protects the 64-bit software stats counters. In
lan9645x_stats_get_stats64, which acts as the ndo_get_stats64 callback, this
lock is acquired using a plain spin_lock. The code comment notes this function
is called in atomic context (such as from NET_RX_SOFTIRQ via dev_get_stats).
Concurrently, process context paths like lan9645x_stats_get_ethtool_stats and
lan9645x_check_stats_work also acquire sw_lock using plain spin_lock. Because
plain spin_lock does not disable softirqs, if a softirq preempts the execution
on a CPU while process context holds sw_lock, the softirq will spin
indefinitely waiting for the lock.
Should the process-context acquisitions use spin_lock_bh instead?
[ ... ]
> +int lan9645x_stats_init(struct lan9645x *lan9645x)
> +{
> + const struct lan9645x_view_stats *vs;
> + struct lan9645x_stats *stats;
> + int err, i;
> +
> + lan9645x->stats = devm_kzalloc(lan9645x->dev, sizeof(*stats),
> + GFP_KERNEL);
Will this memory be leaked when the DSA switch unbinds and rebinds?
The lan9645x_stats_init function allocates lan9645x->stats, vstat->cnts, and
vstat->buf using devm_kzalloc and devm_kcalloc. This function is called from
lan9645x_setup, which is the DSA .setup callback.
The DSA tree lifecycle is independent of the platform device lifecycle. If the
master netdevice is unbound and bound again, the DSA switch's .teardown and
.setup callbacks will be invoked multiple times. Because .teardown cannot free
devres-managed memory, the memory tied to the platform device will be leaked
on every setup cycle until the switch's platform device itself is destroyed.
Could this use standard kzalloc allocations paired with kfree in teardown?
^ permalink raw reply
* Re: [PATCH net v3 04/11] list: Move on_list_rcu() to list.h and add on_list() also
From: Linus Torvalds @ 2026-03-29 20:01 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Howells, netdev, Marc Dionne, David S. Miller, Eric Dumazet,
Paolo Abeni, linux-afs, linux-kernel, Mathieu Desnoyers,
John Johansen, Minas Harutyunyan, Simon Horman, apparmor,
linux-usb, stable
In-Reply-To: <20260329121208.6092419d@kernel.org>
On Sun, 29 Mar 2026 at 12:12, Jakub Kicinski <kuba@kernel.org> wrote:
>
> Could someone with sufficient weight to their name ack this?
I don't particularly like it. I think the name is too generic, and
it's all wrong anyway. Whether something is on a list or not ends up
being much too specific to the use-case, and I do *not* see a huge
advantage to a helper function that just wraps "list_empty()" with
another name that is actually *less* descriptive.
So no. NAK.
As you mention, the RCU version at least does something else, but
honestly, it looks pretty damn questionable too. And no, it does not
work for non-RCU lists in any kind of generic sense, since iut's
perfectly valid to remove list entries without poisoning them.,
For example, some places that want a simple "am I on a list" will use
__list_del_clearprev(), which does *not* poison the prev pointer, but
just clears it instead. And then the "am I on a list" is just checking
prev for NULL or not.
In other words: all of this is wrong. Whether you are on a list or not
is simply not a generic operation. It depends on the user.
The name is also *MUCH* too generic anyway.
Linus
^ permalink raw reply
* AW: [BUG] net: ethernet: cortina: gemini: skb leak in gmac_rx() causes kernel lockup under sustained RX load
From: Andreas Haarmann-Thiemann @ 2026-03-29 20:01 UTC (permalink / raw)
To: 'Linus Walleij'; +Cc: ulli.kroll, netdev, linux-arm-kernel
In-Reply-To: <CAD++jLmVPCL6p3jcdg1y_w=Zij6oVdWTvSmQFjWLn4yRJL4g=w@mail.gmail.com>
Hello Linus,
thank you for the confirmation!
Here is my Signed-off-by:
Signed-off-by: Andreas Haarmann-Thiemann <eitschman@nebelreich.de>
Please feel free to create the patch from the inline code.
Best regards,
Andreas Haarmann-Thiemann
-----Ursprüngliche Nachricht-----
Von: Linus Walleij <linusw@kernel.org>
Gesendet: Sonntag, 29. März 2026 20:54
An: Andreas Haarmann-Thiemann <eitschman@nebelreich.de>
Cc: ulli.kroll@googlemail.com; netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org
Betreff: Re: [BUG] net: ethernet: cortina: gemini: skb leak in gmac_rx() causes kernel lockup under sustained RX load
Hi Andreas,
thanks for digging into this, I have wondered why this happens for a long time but I'm not the best net developer myself.
On Sun, Mar 29, 2026 at 12:05 PM Andreas Haarmann-Thiemann <eitschman@nebelreich.de> wrote:
> diff --git a/drivers/net/ethernet/cortina/gemini.c
> b/drivers/net/ethernet/cortina/gemini.c
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c
>
> @@ -1491,6 +1491,10 @@ static int gmac_rx(struct napi_struct *napi, int budget)
> gpage = gmac_get_queue_page(geth, port, mapping + PAGE_SIZE);
> if (!gpage) {
> dev_err(geth->dev,
> "could not find mapping\n");
> + if (skb) {
> + napi_free_frags(&port->napi);
> + skb = NULL;
> + }
> port->stats.rx_dropped++;
> continue;
> }
This looks right to me, can you send a proper patch, or provide your Signed-off-by in this thread so I can create a patch from this inline code?
The kernel process requires a "certificate of origin" i.e. Signed-off-by, described a bit down in this document:
https://docs.kernel.org/process/submitting-patches.html
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH net-next v2 4/6] enic: make enic_dev_enable/disable ref-counted
From: Jakub Kicinski @ 2026-03-29 20:13 UTC (permalink / raw)
To: devnull+satishkh.cisco.com
Cc: Jakub Kicinski, satishkh, andrew+netdev, davem, edumazet, pabeni,
netdev, linux-kernel
In-Reply-To: <20260325-enic-sriov-v2-prep-v2-4-f72cf716ebfa@cisco.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.
---
enic: make enic_dev_enable/disable ref-counted
Both the data path (ndo_open/ndo_stop) and the upcoming admin channel
need to enable and disable the vNIC device independently. Without
reference counting, closing the admin channel while the netdev is up
would inadvertently disable the entire device.
[ ... ]
> diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.c b/drivers/net/ethernet/cisco/enic/enic_dev.c
> index 2cbae7c6cc3d6..659787f73cf18 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_dev.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_dev.c
> @@ -131,10 +131,13 @@ int enic_dev_set_ig_vlan_rewrite_mode(struct enic *enic)
>
> int enic_dev_enable(struct enic *enic)
> {
> - int err;
> + int err = 0;
>
> spin_lock_bh(&enic->devcmd_lock);
> - err = vnic_dev_enable_wait(enic->vdev);
> + if (enic->enable_count == 0)
> + err = vnic_dev_enable_wait(enic->vdev);
> + if (!err)
> + enic->enable_count++;
> spin_unlock_bh(&enic->devcmd_lock);
>
> return err;
> @@ -142,10 +145,16 @@ int enic_dev_enable(struct enic *enic)
>
> int enic_dev_disable(struct enic *enic)
> {
> - int err;
> + int err = 0;
>
> spin_lock_bh(&enic->devcmd_lock);
> - err = vnic_dev_disable(enic->vdev);
> + if (enic->enable_count == 0) {
> + spin_unlock_bh(&enic->devcmd_lock);
> + return 0;
> + }
> + enic->enable_count--;
> + if (enic->enable_count == 0)
> + err = vnic_dev_disable(enic->vdev);
> spin_unlock_bh(&enic->devcmd_lock);
>
> return err;
If vnic_dev_enable_wait() fails, enable_count remains 0. However, the data
path caller enic_open() ignores the return value of enic_dev_enable() and
unconditionally returns 0:
drivers/net/ethernet/cisco/enic/enic_main.c:enic_open() {
...
enic_dev_enable(enic);
for (i = 0; i < enic->intr_count; i++)
vnic_intr_unmask(&enic->intr[i]);
...
return 0;
}
Can this cause the device reference count to become unbalanced?
If enic_dev_enable() fails during enic_open(), the interface is marked
logically up but enable_count remains 0. If enic_stop() is called later,
enic_dev_disable() will return early since the count is 0, skipping the
hardware disable command.
Additionally, if the new admin channel calls enic_dev_enable() while
the device is in this state, it will increment the count from 0 to 1.
When the admin channel later calls enic_dev_disable(), the count will
drop back to 0 and issue vnic_dev_disable(), incorrectly disabling the
hardware while the netdev data path is still up.
Should enic_open() check the return value of enic_dev_enable() and
handle failures appropriately, or should the enable_count be incremented
regardless of the hardware command success to keep the logical states
aligned?
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH net-next v2 6/6] enic: detect admin channel resources for SR-IOV
From: Jakub Kicinski @ 2026-03-29 20:13 UTC (permalink / raw)
To: devnull+satishkh.cisco.com
Cc: Jakub Kicinski, satishkh, andrew+netdev, davem, edumazet, pabeni,
netdev, linux-kernel
In-Reply-To: <20260325-enic-sriov-v2-prep-v2-6-f72cf716ebfa@cisco.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.
---
enic: detect admin channel resources for SR-IOV
This commit checks for the presence of admin channel BAR resources during
resource discovery and sets has_admin_channel when all four are available.
It also adds admin WQ, RQ, CQ and INTR fields to struct enic.
> diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
> index 67fd780b1fa1c..08472420f3a1f 100644
> --- a/drivers/net/ethernet/cisco/enic/enic.h
> +++ b/drivers/net/ethernet/cisco/enic/enic.h
> @@ -289,6 +289,13 @@ struct enic {
> u8 rss_key[ENIC_RSS_LEN];
> struct vnic_gen_stats gen_stats;
> enum ext_cq ext_cq;
> +
> + /* Admin channel resources for SR-IOV MBOX */
> + bool has_admin_channel;
> + struct vnic_wq admin_wq;
> + struct vnic_rq admin_rq;
> + struct vnic_cq admin_cq[2];
> + struct vnic_intr admin_intr;
> };
[ ... ]
> diff --git a/drivers/net/ethernet/cisco/enic/enic_res.c b/drivers/net/ethernet/cisco/enic/enic_res.c
> index bbd3143ed73e7..5ca3e5aec7f93 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_res.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_res.c
> @@ -205,10 +205,17 @@ void enic_get_res_counts(struct enic *enic)
> enic->cq_count = enic->cq_avail;
> enic->intr_count = enic->intr_avail;
>
> + enic->has_admin_channel =
> + vnic_dev_get_res_count(enic->vdev, RES_TYPE_ADMIN_WQ) >= 1 &&
> + vnic_dev_get_res_count(enic->vdev, RES_TYPE_ADMIN_RQ) >= 1 &&
> + vnic_dev_get_res_count(enic->vdev, RES_TYPE_ADMIN_CQ) >= 1 &&
Is it possible for the hardware to provide exactly 1 admin completion queue?
Because struct enic allocates two admin_cq entries:
struct vnic_cq admin_cq[2];
If the firmware only provides one RES_TYPE_ADMIN_CQ, this check will pass,
but later initialization of admin_cq[1] might access memory past the end of
the validated memory-mapped I/O boundary.
Should this check be >= 2, or use ARRAY_SIZE(enic->admin_cq), to ensure
both queues are backed by hardware?
> + vnic_dev_get_res_count(enic->vdev, RES_TYPE_SRIOV_INTR) >= 1;
> +
^ permalink raw reply
* Re: [PATCH net-next v2] declance: Remove IRQF_ONESHOT
From: Maciej W. Rozycki @ 2026-03-29 20:27 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: netdev, linux-mips, Jakub Kicinski, Andrew Lunn, David S. Miller,
Eric Dumazet, Paolo Abeni
In-Reply-To: <alpine.DEB.2.21.2601271739250.40317@angie.orcam.me.uk>
On Tue, 27 Jan 2026, Maciej W. Rozycki wrote:
> > > I need more data to conclude whether this is the right change to make I'm
> > > afraid. Thank you for looking into it though.
> >
> > Fair enough. Would it work for you if we scratch this from net-next and
> > you route this or something else via the mips tree?
>
> No need to, I think I understand the situation now. Surely the comment
> referring IRQF_ONESHOT in arch/mips/dec/ioasic-irq.c needs to be removed,
> but otherwise this is:
>
> Acked-by: Maciej W. Rozycki <macro@orcam.me.uk>
>
> Thank you for clarifying this to me, and doing the clean-up in the first
> place!
I've now got back to it and while preparing the justification for the
removal of the IRQF_ONESHOT recommendation and having looked through
Documentation/core-api/real-time/differences.rst I became stumped and
need a further clarification after all.
I read in the document that:
"However, on a PREEMPT_RT system, interrupts are forced-threaded and no
longer run in hard IRQ context."
and:
"All interrupts are forced-threaded in a PREEMPT_RT system. The exceptions
are interrupts that are requested with the IRQF_NO_THREAD, IRQF_PERCPU, or
IRQF_ONESHOT flags."
-- do I infer correctly that on a PREEMPT_RT system in the absence of any
flags passed to request_irq() the handler requested such as one concerned
here (i.e. lance_dma_merr_int()) will run with interrupts locally enabled
on the CPU?
If so, then either we need to go back and make sure the originating IRQ
line is masked throughout the execution of the handler (and no standard
irq-flow method provides it), or any IOASIC DMA error interrupt handlers,
including this one, have to use the IRQF_NO_THREAD flag instead or the CPU
will hang looping on the interrupt being retriggered at enable time, as in
the absence of masking the interrupt output of the interrupt controller
remains active until the final EOI action. Have I missed anything?
Mind that this is somewhat theoretical, given that declance.c is only for
systems using the MIPS CPU and arch/mips does not enable ARCH_SUPPORTS_RT,
however should it do sometime, I'd rather all the hell didn't break loose.
And there's previous art already as I can see IRQF_NO_THREAD used through
arch/mips, following commit 5a4a4ad851dd ("MIPS: Mark cascade and low
level interrupts IRQF_NO_THREAD").
Maciej
^ permalink raw reply
* Re: [PATCH 1/2] net: qrtr: ns: Limit the maximum server registration per node
From: Jakub Kicinski @ 2026-03-29 20:33 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Simon Horman, Manivannan Sadhasivam, davem, edumazet, pabeni,
linux-arm-msm, netdev, linux-kernel, andersson, yimingqian591,
chris.lew, stable
In-Reply-To: <as3zucfwr4z2x5pxww6ognmqcujkwnhppulm7jquex6fy6sqn5@qa33h5mxxdz7>
On Fri, 27 Mar 2026 15:40:01 +0530 Manivannan Sadhasivam wrote:
> > I am wondering if any bounds are placed on the number of nodes that can be
> > created. And, if not, is this a point of concern from a memory exhaustion
> > perspective?
>
> That's true. I plan to send a followup for that. This series just limits the
> scope in addressing the reported issue.
This series is moot without such limit, tho.
Let's fix it all in one series.
I'll send you the remaining AI feedback.
^ permalink raw reply
* Re: [PATCH 1/2] net: qrtr: ns: Limit the maximum server registration per node
From: Jakub Kicinski @ 2026-03-29 20:33 UTC (permalink / raw)
To: manivannan.sadhasivam
Cc: Jakub Kicinski, davem, edumazet, pabeni, horms, linux-arm-msm,
netdev, linux-kernel, andersson, yimingqian591, chris.lew, mani,
stable
In-Reply-To: <20260325104415.104972-2-manivannan.sadhasivam@oss.qualcomm.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: qrtr: ns: Limit the maximum server registration per node
This commit introduces a limit of 256 server registrations per node to
prevent malicious clients from exhausting memory by flooding NEW_SERVER
messages.
Cc: stable@vger.kernel.org
Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
Reported-by: Yiming Qian <yimingqian591@gmail.com>
> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index 3203b2220860..fb4e8a2d370d 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
[ ... ]
> @@ -229,6 +235,17 @@ static struct qrtr_server *server_add(unsigned int service,
> if (!service || !port)
> return NULL;
>
> + node = node_get(node_id);
Does this per-node limit fully protect against memory exhaustion?
If a remote client spoofs the node ID in the NEW_SERVER message, the
attacker-controlled node_id is passed to server_add(). Because node_get()
unconditionally allocates a new qrtr_node structure when one is not found,
an attacker could continuously send messages with unique node IDs.
Could this result in unbounded node allocations that still exhaust system
memory despite the per-node server limit?
> + if (!node)
> + return NULL;
> +
> + /* Make sure the new servers per port are capped at the maximum value */
> + old = xa_load(&node->servers, port);
> + if (!old && node->server_count >= QRTR_NS_MAX_SERVERS) {
> + pr_err_ratelimited("QRTR client node %u exceeds max server limit!\n", node_id);
> + return NULL;
While this error message is rate-limited, returning NULL here causes
ctrl_cmd_new_server() to return -EINVAL.
In the main packet loop qrtr_ns_worker(), a negative return value triggers
an un-ratelimited error:
if (ret < 0)
pr_err("failed while handling packet from %d:%d",
sq.sq_node, sq.sq_port);
Could a flood of excess server registrations trigger this un-ratelimited
message and cause a console logging storm?
> + }
> +
> srv = kzalloc_obj(*srv);
> if (!srv)
> return NULL;
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH 2/2] net: qrtr: ns: Limit the maximum lookups per socket
From: Jakub Kicinski @ 2026-03-29 20:33 UTC (permalink / raw)
To: manivannan.sadhasivam
Cc: Jakub Kicinski, davem, edumazet, pabeni, horms, linux-arm-msm,
netdev, linux-kernel, andersson, yimingqian591, chris.lew, mani,
stable
In-Reply-To: <20260325104415.104972-3-manivannan.sadhasivam@oss.qualcomm.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: qrtr: ns: Limit the maximum lookups per socket
This commit limits the maximum number of lookups to 64 per socket to prevent
a malicious local client from sending a flood of NEW_LOOKUP messages over the
same socket.
> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index fb4e8a2d370d..707fde809939 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
[ ... ]
> @@ -545,11 +546,24 @@ static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from,
> struct qrtr_node *node;
> unsigned long node_idx;
> unsigned long srv_idx;
> + u8 count = 0;
>
> /* Accept only local observers */
> if (from->sq_node != qrtr_ns.local_node)
> return -EINVAL;
>
> + /* Make sure the client performs only maximum allowed lookups */
> + list_for_each_entry(lookup, &qrtr_ns.lookups, li) {
> + if (lookup->sq.sq_node == from->sq_node &&
> + lookup->sq.sq_port == from->sq_port)
> + count++;
> + }
Does this O(N) traversal of the global qrtr_ns.lookups list create a CPU
exhaustion issue?
If a local attacker opens many sockets and registers the maximum 64 lookups
on each, the global list could grow to tens of thousands of items.
If the attacker then floods the socket with NEW_LOOKUP packets, the
single-threaded qrtr_ns.workqueue worker would be forced to traverse all
elements for every packet, potentially blocking all legitimate QRTR namespace
control traffic.
> +
> + if (count >= QRTR_NS_MAX_LOOKUPS) {
> + pr_err_ratelimited("QRTR client node exceeds max lookup limit!\n");
> + return -ENOSPC;
Does returning -ENOSPC here defeat the ratelimiting?
If the caller qrtr_ns_worker() unconditionally checks for a negative return
value and executes an unratelimited error print, a malicious local client
could still cause a syslog flood by sending continuous NEW_LOOKUP packets
after hitting the limit.
Should this return 0 instead to avoid the unratelimited print in the caller?
> + }
> +
> lookup = kzalloc_obj(*lookup);
> if (!lookup)
> return -ENOMEM;
This isn't a regression introduced by this patch, but does qrtr_ns_remove()
contain a use-after-free during module unload?
The teardown sequence appears to be:
1. cancel_work_sync()
2. destroy_workqueue(qrtr_ns.workqueue)
3. sock_release(qrtr_ns.sock)
If a packet arrives between destroy_workqueue() and sock_release(), the
socket's receive path triggers qrtr_ns_data_ready(). This callback calls
queue_work(qrtr_ns.workqueue, ...), which dereferences the already-freed
workqueue pointer.
Should the socket be shut down or the callback disabled before destroying
the workqueue?
^ permalink raw reply
* Re: [PATCH net 1/2] net/smc: fix potential UAF in smc_pnet_add_ib for ib device
From: Jakub Kicinski @ 2026-03-29 20:35 UTC (permalink / raw)
To: lixiasong1
Cc: Jakub Kicinski, alibuda, dust.li, sidraya, wenjia, mjambigi,
tonylu, guwen, davem, edumazet, pabeni, horms, linux-rdma,
linux-s390, netdev, linux-kernel, yuehaibing, zhangchangzhong,
weiyongjun1
In-Reply-To: <20260325110352.3833570-2-lixiasong1@huawei.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.
---
No issues found.
--
pw-bot: cr
^ 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