* Re: [PATCH net v3 13/14] rxrpc: Fix CHALLENGE packet overqueuing and simplify RESPONSE generation
From: Simon Horman @ 2026-07-20 14:54 UTC (permalink / raw)
To: dhowells
Cc: Simon Horman, netdev, marc.dionne, kuba, davem, edumazet, pabeni,
linux-afs, linux-kernel, stable
In-Reply-To: <20260713081022.2186481-14-dhowells@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.
https://sashiko.dev/#/patchset/20260713081022.2186481-8-dhowells@redhat.com?part=13
---
--- Patch [13]: [PATCH net v3 13/14] rxrpc: Fix CHALLENGE packet overqueuing and simplify RESPONSE generation ---
commit ebc5bb53c96c167e2d7c2726bed42c09fd3a97d1
Author: David Howells <dhowells@redhat.com>
rxrpc: Fix CHALLENGE packet overqueuing and simplify RESPONSE generation
This commit removes the out-of-band queueing mechanism for rxrpc challenge
packets, moving instead to a preemptively supplied application data key.
The event processor now parses the challenge and generates the response
directly using this key.
Link: https://sashiko.dev/#/patchset/20260624163819.3017002-1-dhowells%40redhat.com
> --- a/net/rxrpc/recvmsg.c
> +++ b/net/rxrpc/recvmsg.c
> @@ -405,7 +405,7 @@ int rxrpc_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>
> trace_rxrpc_recvmsg(0, rxrpc_recvmsg_enter, 0);
>
> - if (flags & (MSG_OOB | MSG_TRUNC))
> + if (flags & MSG_TRUNC)
> return -EOPNOTSUPP;
[Severity: Medium]
Does removing MSG_OOB from this check cause out-of-band requests to silently
fall through and return in-band data?
By not returning -EOPNOTSUPP for MSG_OOB, the socket might violate standard
semantics by returning regular data when the caller explicitly requested
out-of-band data.
> --- a/net/rxrpc/rxgk.c
> +++ b/net/rxrpc/rxgk.c
> @@ -936,7 +937,7 @@ static int rxgk_construct_response(struct rxrpc_connection *conn,
> if (IS_ERR(gk))
> return PTR_ERR(gk);
>
> - auth_len = 20 + (4 + appdata->len) + 12 + (1 + 4) * 4;
> + auth_len = 20 + (4 + appdata->datalen) + 12 + (1 + 4) * 4;
[Severity: Critical]
Is auth_len calculated incorrectly here by missing the padding for
appdata->datalen?
It looks like this unpadded length is used to calculate authx_len, which
determines the size of the skb and the scatterlist allocation.
Later, rxgk_construct_authenticator() uses rxgk_pad_out() to correctly
round up the written bytes:
ret = rxgk_pad_out(response, appdata->datalen, offset);
Since the written padded size is larger than the unpadded alloc_len
passed to rxgk_encrypt_authenticator(), will this cause the crypto layer
to write past the end of the scatterlist and corrupt memory?
[ ... ]
> @@ -1011,24 +1012,36 @@ static int rxgk_construct_response(struct rxrpc_connection *conn,
> * Respond to a challenge packet.
> */
> static int rxgk_respond_to_challenge(struct rxrpc_connection *conn,
> - struct sk_buff *challenge,
> - struct krb5_buffer *appdata)
> + struct sk_buff *challenge)
> {
> - _enter("{%d,%x}", conn->debug_id, key_serial(conn->key));
> + struct user_key_payload dummy = {}, *appdata = &dummy;
> + int ret;
> +
> + _enter("{%d,%u,%x,%x}",
> + conn->debug_id, conn->service_id,
> + key_serial(conn->key), key_serial(conn->bundle->app_data));
>
> if (key_validate(conn->key) < 0)
> return rxrpc_abort_conn(conn, NULL, RXGK_EXPIRED, -EPROTO,
> rxgk_abort_chall_key_expired);
>
> - return rxgk_construct_response(conn, challenge, appdata);
> -}
> + if (conn->bundle->app_data) {
> + rcu_read_lock();
> + appdata = (struct user_key_payload *)
> + user_key_payload_rcu(conn->bundle->app_data);
> + if (appdata && !refcount_inc_not_zero(&appdata->ref))
> + appdata = NULL;
> + rcu_read_unlock();
[Severity: Critical]
Is it safe to blindly cast the payload of conn->bundle->app_data to
a struct user_key_payload here?
If a non-user key like a keyring or logon key was somehow attached to
the call, this cast will interpret the alien key payload as a
user_key_payload. For example, if rxrpc_sendmsg_cmsg() in
net/rxrpc/sendmsg.c has an inverted type check that inadvertently accepts
non-user keys:
if (key_ref_to_ptr(key)->type == &key_type_user) {
key_ref_put(key);
return -EINVAL;
}
Calling refcount_inc_not_zero() on a mismatched type could corrupt memory
by incrementing whatever happens to be at that offset. Furthermore, reading
the data length from this corrupted structure could leak arbitrary kernel
memory into the network via the response packet.
^ permalink raw reply
* Re: [PATCH net v3 14/14] rxrpc: Remove OOB challenge/response code
From: Simon Horman @ 2026-07-20 14:56 UTC (permalink / raw)
To: dhowells
Cc: Simon Horman, netdev, marc.dionne, kuba, davem, edumazet, pabeni,
linux-afs, linux-kernel, stable
In-Reply-To: <20260713081022.2186481-15-dhowells@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.
https://sashiko.dev/#/patchset/20260713081022.2186481-8-dhowells@redhat.com?part=14
---
--- Patch [14]: [PATCH net v3 14/14] rxrpc: Remove OOB challenge/response code ---
commit fd3c68debe8844700015aad0c42af18084298daa
Author: David Howells <dhowells@redhat.com>
rxrpc: Remove OOB challenge/response code
This commit removes all the OOB communication stuff along with the UAPI
elements related to them. The reason for this is that there is no actual
limit on the capacity of the CHALLENGE queue, which could be abused
remotely, and it is not actually necessary to generate the application
data in response to the CHALLENGE.
Link: https://sashiko.dev/#/patchset/20260624163819.3017002-1-dhowells%40redhat.com
> diff --git a/include/uapi/linux/rxrpc.h b/include/uapi/linux/rxrpc.h
> index bcdfdf9c67a14..7d6a501519311 100644
> --- a/include/uapi/linux/rxrpc.h
> +++ b/include/uapi/linux/rxrpc.h
> @@ -58,11 +58,6 @@ enum rxrpc_cmsg_type {
> RXRPC_TX_LENGTH = 12, /* -s-: Total length of Tx data */
> RXRPC_SET_CALL_TIMEOUT = 13, /* -s-: Set one or more call timeouts */
> RXRPC_CHARGE_ACCEPT = 14, /* Ss-: Charge the accept pool with a user call ID */
> - RXRPC_OOB_ID = 15, /* -sr: OOB message ID */
> - RXRPC_CHALLENGED = 16, /* C-r: Info on a received CHALLENGE */
> - RXRPC_RESPOND = 17, /* Cs-: Respond to a challenge */
> - RXRPC_RESPONDED = 18, /* S-r: Data received in RESPONSE */
> - RXRPC_RESP_RXGK_APPDATA = 19, /* Cs-: RESPONSE: RxGK app data to include */
> RXRPC_RESPONSE_APPDATA = 20, /* Cs-: User key holding app data for RESPONSE */
> RXRPC__SUPPORTED
> };
[Severity: Critical]
Does removing these constants from the UAPI header violate the kernel's
policy against breaking userspace?
Any userspace program that includes linux/rxrpc.h and references the
removed RXRPC_OOB_ID or related constants will now fail to compile.
[Severity: Critical]
Can this sequence cause memory corruption and an out-of-bounds information
leak via RXRPC_RESPONSE_APPDATA?
Looking at the remaining sendmsg code in net/rxrpc/sendmsg.c, an inverted
check incorrectly rejects user keys but accepts the keyring:
net/rxrpc/sendmsg.c:rxrpc_sendmsg_cmsg() {
...
if (key_ref_to_ptr(key)->type == &key_type_user) {
key_ref_put(key);
return -EINVAL;
}
...
}
If a local user calls sendmsg with RXRPC_RESPONSE_APPDATA to attach a
keyring key, receiving a CHALLENGE packet triggers
rxgk_respond_to_challenge(), which blindly casts the keyring payload to a
user_key_payload:
net/rxrpc/rxgk.c:rxgk_respond_to_challenge() {
...
appdata = (struct user_key_payload *)
user_key_payload_rcu(conn->bundle->app_data);
if (appdata && !refcount_inc_not_zero(&appdata->ref))
...
}
This yields a pointer to name_link, and incrementing appdata->ref (at
offset 16) corrupts the keyring's keys.root pointer.
Finally, rxgk_construct_authenticator() reads datalen and data from this
corrupted offset and copies out-of-bounds kernel memory into the network
response:
net/rxrpc/rxgk.c:rxgk_construct_authenticator() {
...
a.appdata_len = htonl(appdata->datalen);
if (appdata->datalen) {
ret = skb_store_bits(response, offset, appdata->data,
appdata->datalen);
...
}
Could this allow an unprivileged user to trigger memory corruption and
kernel panic through an AF_RXRPC socket?
> diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
> index 9eb6ca3c5ebf5..9d5ce1c7ad0d2 100644
> --- a/net/rxrpc/af_rxrpc.c
> +++ b/net/rxrpc/af_rxrpc.c
[ ... ]
> @@ -710,23 +710,7 @@ static int rxrpc_setsockopt(struct socket *sock, int level, int optname,
>
> case RXRPC_MANAGE_RESPONSE:
> ret = -EINVAL;
> - if (optlen != sizeof(unsigned int))
> - goto error;
> - ret = -EISCONN;
> - if (rx->sk.sk_state != RXRPC_UNBOUND)
> - goto error;
> - ret = copy_safe_from_sockptr(&val, sizeof(val),
> - optval, optlen);
> - if (ret)
> - goto error;
> - ret = -EINVAL;
> - if (val > 1)
> - goto error;
> - if (val)
> - set_bit(RXRPC_SOCK_MANAGE_RESPONSE, &rx->flags);
> - else
> - clear_bit(RXRPC_SOCK_MANAGE_RESPONSE, &rx->flags);
> - goto success;
> + goto error;
>
> default:
> break;
[Severity: Critical]
Does this change break backwards compatibility for existing applications?
An application calling setsockopt with RXRPC_MANAGE_RESPONSE, which
previously succeeded, will now unconditionally receive -EINVAL.
> diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
> index 20c10428a50e0..1c6f4a279034f 100644
> --- a/net/rxrpc/ar-internal.h
> +++ b/net/rxrpc/ar-internal.h
[ ... ]
> @@ -310,13 +305,6 @@ struct rxrpc_security {
> bool (*validate_challenge)(struct rxrpc_connection *conn,
> struct sk_buff *skb);
>
> - /* Fill out the cmsg for recvmsg() to pass on a challenge to userspace.
> - * The security class gets to add additional information.
> - */
> - int (*challenge_to_recvmsg)(struct rxrpc_connection *conn,
> - struct sk_buff *challenge,
> - struct msghdr *msg);
> -
> /* Parse sendmsg() control message and respond to challenge. */
> int (*sendmsg_respond_to_challenge)(struct sk_buff *challenge,
> struct msghdr *msg);
[Severity: Low]
Is the sendmsg_respond_to_challenge function pointer left behind as dead
code?
It appears the OOB feature removal leaves this callback, along with
notify_oob in struct rxrpc_kernel_ops (include/net/af_rxrpc.h), the
rxrpc_skb_put_oob trace constant in include/trace/events/rxrpc.h, and dummy
implementations like none_sendmsg_respond_to_challenge() in
net/rxrpc/insecure.c.
^ permalink raw reply
* Re: [PATCH net v3 2/2] sctp: auth: verify auth requirement when auth_chunk is NULL
From: Xin Long @ 2026-07-20 15:01 UTC (permalink / raw)
To: luoqing
Cc: marcelo.leitner, davem, edumazet, kuba, pabeni, horms, linux-sctp,
netdev, linux-kernel
In-Reply-To: <20260720093116.1266202-2-l1138897701@163.com>
On Mon, Jul 20, 2026 at 5:32 AM luoqing <l1138897701@163.com> wrote:
>
> From: Qing Luo <luoqing@kylinos.cn>
>
> sctp_auth_chunk_verify() currently returns true unconditionally
> when chunk->auth_chunk is NULL, which means authentication is
> silently skipped. This is incorrect in two scenarios:
>
> 1. skb_clone() failed in the BH receive path, leaving auth_chunk
> NULL. Although the previous fix avoids setting auth=1 in this
> case, the chunk can still reach sctp_auth_chunk_verify() via
> sctp_endpoint_bh_rcv() where asoc is NULL for new connections,
> bypassing the early sctp_auth_recv_cid() check.
>
> 2. No AUTH chunk precedes COOKIE-ECHO in the packet. In this case
> skb_clone() is never called and auth_chunk remains NULL. Again,
> in sctp_endpoint_bh_rcv() the early check cannot catch this
> because asoc is NULL and sctp_auth_recv_cid() returns 0.
>
> Fix by checking sctp_auth_recv_cid() when auth_chunk is NULL:
> if authentication is required for this chunk type, return false
> to drop the chunk; otherwise, continue normally.
>
> Fixes: bbd0d59809f9 ("[SCTP]: Implement the receive and verification of AUTH chunk")
> Signed-off-by: Qing Luo <luoqing@kylinos.cn>
> ---
> net/sctp/sm_statefuns.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index d23d935e128e..89ed618b1de3 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -642,7 +642,7 @@ static bool sctp_auth_chunk_verify(struct net *net, struct sctp_chunk *chunk,
> struct sctp_chunk auth;
>
> if (!chunk->auth_chunk)
> - return true;
> + return !sctp_auth_recv_cid(chunk->chunk_hdr->type, asoc);
>
> /* SCTP-AUTH: auth_chunk pointer is only set when the cookie-echo
> * is supposed to be authenticated and we have to do delayed
> --
> 2.25.1
> >> A better fix would be:
> >>
> >> Add a check in sctp_auth_chunk_verify() at the point where the COOKIE-ECHO
> >> chunk is actually being processed:
> >>
> >>
> >> if (!chunk->auth_chunk)
> >> return !sctp_auth_recv_cid(chunk->chunk_hdr->type, asoc);
> >>
> >> This ensures that if chunk->auth_chunk is missing while authentication is
> >> required for the COOKIE-ECHO chunk, the verification fails and the chunk is
> >> dropped. Otherwise, when authentication is not required, processing can
> >> continue normally.
> >>
> >> Please give it a try.
> >>
> > Also, please add a extra Fixes tag in your next post:
> >
> > Fixes: bbd0d59809f9 ("[SCTP]: Implement the receive and verification
> > of AUTH chunk")
> >
> > which introduces chunk->auth_chunk and calls skb_clone() in
> > sctp_endpoint_bh_rcv().
> Hi,
>
> Thanks for the review. I’ve reworked the fix into two patches:
>
> Patch 1/2: In sctp_assoc_bh_rcv() and sctp_endpoint_bh_rcv(), only set chunk->auth = 1 when skb_clone() succeeds.
>
> Patch 2/2: In sctp_auth_chunk_verify(), when auth_chunk is NULL, check sctp_auth_recv_cid() to decide whether authentication is required. This covers both cases from the review.
>
> I’d like to discuss whether Patch 1 is necessary. Patch 2 alone is sufficient for correctness — even with auth == 1 and auth_chunk == NULL, Patch 2 catches it at the verification point. Patch 1 only provides semantic cleanliness (not setting auth = 1 without a valid auth_chunk), but closes no additional gap.
>
> Should I keep Patch 1 as a defensive cleanup, or drop it and submit only Patch 2?
>
I agree that adding the chunk->auth_chunk check makes the logic clearer.
However, the intention behind skipping chunk->auth = 1 is to drop the
packet earlier. During the normal handshake path (sctp_endpoint_bh_rcv()),
asoc is NULL, and sctp_auth_recv_cid() always returns 0. As a result, not
setting chunk->auth = 1 does not actually achieve the intended effect in
this case.
Given that, I think it would be better to simply break the loop in both
functions:
if (!chunk->auth_chunk)
break;
chunk->auth = 1;
Can you move forward with the 2/2 patch only for this issue? and post the
1/2 patch to 'net-next' as an improvement.
Thanks.
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH net v2] i40e: xsk: fix multi-buffer XDP_PASS skb construction
From: Alexander Lobakin @ 2026-07-20 15:03 UTC (permalink / raw)
To: Chenguang Zhao
Cc: anthony.l.nguyen, przemyslaw.kitszel, andrew+netdev, davem,
edumazet, kuba, pabeni, intel-wired-lan, netdev, Chenguang Zhao
In-Reply-To: <20260717012416.168107-1-chenguang.zhao@linux.dev>
From: Chenguang Zhao <chenguang.zhao@linux.dev>
Date: Fri, 17 Jul 2026 09:24:16 +0800
> From: Chenguang Zhao <zhaochenguang@kylinos.cn>
>
> When AF_XDP ZC receives a multi-buffer frame and XDP returns XDP_PASS,
> i40e_construct_skb_zc() copied frags incorrectly: memcpy used
> skb_frag_page() (page metadata) and __skb_fill_page_desc_noacc() was
> given a virtual address instead of a struct page *.
>
> Drop the custom helper and use xdp_build_skb_from_zc() instead. On
> failure, free the xdp buff in the caller. Push the Ethernet header
> back before eth_skb_pad()/i40e_process_skb_fields() because
> xdp_build_skb_from_zc() already called eth_type_trans().
>
> Fixes: 1c9ba9c14658 ("i40e: xsk: add RX multi-buffer support")
> Signed-off-by: Chenguang Zhao <zhaochenguang@kylinos.cn>
Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
One nit below tho.
[...]
> @@ -372,14 +309,20 @@ static void i40e_handle_xdp_result_zc(struct i40e_ring *rx_ring,
> * BIT(I40E_RXD_QW1_ERROR_SHIFT). This is due to that
> * SBP is *not* set in PRT_SBPVSI (default not set).
> */
> - skb = i40e_construct_skb_zc(rx_ring, xdp_buff);
> + skb = xdp_build_skb_from_zc(xdp_buff);
> if (!skb) {
> + xsk_buff_free(xdp_buff);
> rx_ring->rx_stats.alloc_buff_failed++;
> *rx_packets = 0;
> *rx_bytes = 0;
> return;
> }
>
> + /* xdp_build_skb_from_zc() already ran eth_type_trans();
> + * restore the header for eth_skb_pad()/process_skb_fields().
> + */
The netdev rules prefer generic comment style over what we used in the
past for some time already. I.e.
/*
* xdp_build_skb_from_zc() ...
* restore ...
*/
> + __skb_push(skb, skb->data - skb_mac_header(skb));
> +
> if (eth_skb_pad(skb)) {
> *rx_packets = 0;
> *rx_bytes = 0;
Thanks,
Olek
^ permalink raw reply
* Re: [Intel-wired-lan] [PATCH] ice: parser: use array_size() for table allocation
From: Alexander Lobakin @ 2026-07-20 15:05 UTC (permalink / raw)
To: Weimin Xiong
Cc: Tony Nguyen, Przemek Kitszel, intel-wired-lan, netdev,
linux-kernel, Andrew Lunn, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
In-Reply-To: <20260716025100.118145-1-xiongwm2026@163.com>
From: Weimin Xiong <xiongwm2026@163.com>
Date: Thu, 16 Jul 2026 10:51:00 +0800
> Use array_size() when calculating the parser table allocation size so an
> overflow in the firmware-provided item dimensions is detected before
> allocation. Include the overflow helpers explicitly instead of relying on
> an indirect include.
>
> Signed-off-by: Weimin Xiong <xiongwm2026@163.com>
> ---
> diff --git a/drivers/net/ethernet/intel/ice/ice_parser.c b/drivers/net/ethernet/intel/ice/ice_parser.c
> index f8e69630f..c109d3c32 100644
> --- a/drivers/net/ethernet/intel/ice/ice_parser.c
> +++ b/drivers/net/ethernet/intel/ice/ice_parser.c
> @@ -1,6 +1,8 @@
> // SPDX-License-Identifier: GPL-2.0
> /* Copyright (C) 2024 Intel Corporation */
>
> +#include <linux/overflow.h>
This is redundant.
> +
> #include "ice_common.h"
>
> struct ice_pkg_sect_hdr {
> @@ -102,7 +104,7 @@ ice_parser_create_table(struct ice_hw *hw, u32 sect_type,
> if (!seg)
> return ERR_PTR(-EINVAL);
>
> - table = kzalloc(item_size * length, GFP_KERNEL);
> + table = kzalloc(array_size(item_size, length), GFP_KERNEL);
kcalloc(length, item_size) if you want to make it _proper_.
array_size() is used only when no array allocation helpers are
available, e.g. for dma_alloc_coherent(). For regular slab allocations,
we have a whole bunch of different array allocation helpers.
> if (!table)
> return ERR_PTR(-ENOMEM);
Thanks,
Olek
^ permalink raw reply
* Re: [PATCH net-next] net: stmmac: Simplify ioctl handling
From: Vadim Fedorenko @ 2026-07-20 15:17 UTC (permalink / raw)
To: Andrew Lunn, Maxime Chevallier
Cc: Andrew Lunn, Jakub Kicinski, davem, Eric Dumazet, Paolo Abeni,
Simon Horman, Maxime Coquelin, Alexandre Torgue, Russell King,
thomas.petazzoni, Alexis Lothoré, netdev, linux-kernel,
linux-arm-kernel, linux-stm32
In-Reply-To: <ae82997f-537e-4a2d-abce-d163fcf9d868@lunn.ch>
On 19.07.2026 17:13, Andrew Lunn wrote:
>> Looking at this, I'm wondering if we can't just get rid of SIOCSHWTSTAMP
>> handling in phy_mii_ioctl(). Looks like we can ?
>
> I'm not sure about that. We need Richards input.
>
> The code in phy_mii_ioctl() allows the MAC to be bypassed, it goes
> straight to a PHY based stamper. It could be the MAC has no idea the
> PHY has this capability, so it has not implemented the .ndo?
>
> It might be we need to hoist the code from phy_mii_ioctl() into
> dev_{sg}et_hwtstamp()?
Hi Andrew!
I think I've converted all phy drivers while removing support for
SIOCSHWTSTAMP/SIOCGHWTSTAMP from netdev ioctl. I believe it's impossible right
now to reach SIOCSHWTSTAMP path of phy_mii_ioctl via ioctl on net device.
Is it possible to have ioctl on phy device directly without involving netdev?
^ permalink raw reply
* Re: [PATCH net 2/7] selftests: openvswitch: add config file
From: Matthieu Baerts @ 2026-07-20 15:24 UTC (permalink / raw)
To: Aaron Conole
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Shuah Khan, netdev, linux-kselftest, linux-kernel,
Eelco Chaudron, Ilya Maximets, dev
In-Reply-To: <f7t7bmppvpu.fsf@redhat.com>
On 20/07/2026 16:43, Aaron Conole wrote:
> Matthieu Baerts <matttbe@kernel.org> writes:
>
>> Hi Aaron,
>>
>> Thank you for the review!
>>
>> On 16/07/2026 22:00, Aaron Conole wrote:
>>> Aaron Conole <aconole@redhat.com> writes:
>>>> "Matthieu Baerts (NGI0)" <matttbe@kernel.org> writes:
>>>>
>>>>> The kselftests doc mentions that a config file should be present "if a
>>>>> test needs specific kernel config options enabled". This selftest
>>>>> requires some kernel config, but no config file was provided.
>>>>>
>>>>> We could say that a sub-target could use the parent's config file, but
>>>>> the kselftests doc doesn't mention anything about that. Plus the
>>>>> net/openvswitch target is the only net target without a config file.
>>>>
>>>> We've been operating on that assumption from the openvswitch side, but
>>>> it's true that isn't explicitly documented anywhere, and I guess it
>>>> isn't officially supported in the kselftest framework. I guess we'll
>>>> need to keep updating this config as we add tests for things like SCTP,
>>>> and others, and maybe that's a good thing like we can add a comment
>>>> describing which tests take which configs.
>>>>
>>>> The downside is for most of the OVS testing we use the NIPA scripts
>>>> and those 'inherit' the parent config, so it would be a change on our
>>>> side from the development standpoint (but probably something we should
>>>> have been doing from the beginning).
>>>>
>>>> That said, would it be worth also exploring the 'cascading
>>>> configuration' support? It seems like a useful feature, but maybe it
>>>> should be a separate discussion. I ask because of how OVS interacts
>>>> with the networking stack as an 'alternative bridge' so-to-speak, I do
>>>> worry about having to duplicate lots of configurations between the two
>>>> as we expand the test coverage on OVS side.
>>
>> It is not clear to me what you are using on your side, but I guess it
>> should be doable to modify some scripts to merge this new config file
>> and the net one in your case.
>
> This doesn't seem to break the NIPA case (at least I can see the
> 'contest test' output is not SKIP, but PASS for the openvswitch.sh).
Note that currently, the net/openvswitch target on NIPA is executed on
the same runner as the net one. In other words, the config files are
merged, so at the end, my patch shouldn't change anything with the way
NIPA is currently deployed (but that could change).
> I'm not sure about the suggestion to change the script in
> tools/testing/selftests/net/openvswitch/openvswitch.sh to do a config
> merge. It isn't out-of-tree or anything, and seems like that would
> create confusing git state (and not desirable from my end) - the
> upstream test would leave a modified, tracked file. Was that your
> suggestion? Or do you mean NIPA itself (but I think it already just
> uses the top-level config).
No, sorry, I thought my modification would have caused issues with other
tests on your side. Good to hear it is not.
> The config change itself looks fine to me.
Thank you!
>>>>> Here is a new config file, which is a trimmed version of the net one,
>>>>> with hopefully the minimal required kconfig on top of 'make defconfig'.
>>>>
>>>> Should this also remove the OVS configs from the upper level since there
>>>> shouldn't be OVS tests executing there (ie: CONFIG_OPENVSWITCH*)?
>>>
>>> Actually, forget this part. The P-MTU tests in pmtu.sh use ovs to
>>> create a datapath through OVS. So these configurations need to stay at
>>> the top level as well.
>>
>> Indeed. I don't know these tests well, but maybe this P-MTU selftest
>> should move to the net/openvswitch target?
>
> Well, it is testing path MTU across multiple topologies, so it isn't
> really an openvswitch specific test. Just one that happens to use
> openvswitch in one of the test cases.
OK, it is fine there then.
Note that the openvswitch test could also be moved to the 'net' one. It
looks like the sub-targets are often created when there are multiple
tests linked to one sub-subsystem, and also because the 'net' config is
quite big, and it takes time to compile all that. But I'm not pushing
for one or the other, I just wanted to be able to build all targets in
small containers :)
Cheers,
Matt
^ permalink raw reply
* Re: [PATCH net v3] sctp: socket: remove unused 'err' parameter from sctp_skb_recv_datagram
From: Xin Long @ 2026-07-20 15:25 UTC (permalink / raw)
To: David Laight
Cc: luoqing, jedrzej.jagielski, davem, edumazet, horms, kuba,
linux-kernel, linux-sctp, luoqing, marcelo.leitner, netdev,
pabeni
In-Reply-To: <20260717191913.490e5d80@pumpkin>
On Fri, Jul 17, 2026 at 2:19 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Fri, 17 Jul 2026 11:10:21 -0400
> Xin Long <lucien.xin@gmail.com> wrote:
>
> > On Fri, Jul 17, 2026 at 4:21 AM luoqing <l1138897701@163.com> wrote:
> > >
> > > From: luoqing <luoqing@kylinos.cn>
> > >
> > > The 'err' parameter in sctp_skb_recv_datagram() is never used by any
> > > of its callers. Both sctp_recvmsg() and sctp_ulpevent_read_nxtinfo()
> > > pass the address of a local variable but never check its value after
> > > the function returns, rendering the parameter completely useless.
> > >
> > > Remove the unused parameter to simplify the function signature and
> > > eliminate dead code.
> > >
> > > Signed-off-by: luoqing <luoqing@kylinos.cn>
> > > ---
> > > include/net/sctp/sctp.h | 2 +-
> > > net/sctp/socket.c | 10 +++-------
> > > net/sctp/ulpevent.c | 3 +--
> > > 3 files changed, 5 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> > > index d50c27812504..b86d50d6b146 100644
> > > --- a/include/net/sctp/sctp.h
> > > +++ b/include/net/sctp/sctp.h
> > > @@ -97,7 +97,7 @@ void sctp_sock_rfree(struct sk_buff *skb);
> > >
> > > extern struct percpu_counter sctp_sockets_allocated;
> > > int sctp_asconf_mgmt(struct sctp_sock *, struct sctp_sockaddr_entry *);
> > > -struct sk_buff *sctp_skb_recv_datagram(struct sock *, int, int *);
> > > +struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags);
> > >
> > > typedef int (*sctp_callback_t)(struct sctp_endpoint *, struct sctp_transport *, void *);
> > > void sctp_transport_walk_start(struct rhashtable_iter *iter);
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index c7b9e325ec1c..3804382d78e0 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -2123,7 +2123,7 @@ static int sctp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> > > goto out;
> > > }
> > >
> > > - skb = sctp_skb_recv_datagram(sk, flags, &err);
> > > + skb = sctp_skb_recv_datagram(sk, flags);
> > > if (!skb)
> > > goto out;
> > >
> > > @@ -9082,7 +9082,7 @@ static int sctp_wait_for_packet(struct sock *sk, int *err, long *timeo_p)
> > > * Note: This is pretty much the same routine as in core/datagram.c
> > > * with a few changes to make lksctp work.
> > > */
> > > -struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags, int *err)
> > > +struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags)
> > > {
> > > int error;
> > > struct sk_buff *skb;
> > > @@ -9120,17 +9120,13 @@ struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags, int *err)
> > > if (sk->sk_shutdown & RCV_SHUTDOWN)
> > > break;
> > >
> > > -
> > > /* User doesn't want to wait. */
> > > error = -EAGAIN;
> > > if (!timeo)
> > > goto no_packet;
> > > - } while (sctp_wait_for_packet(sk, err, &timeo) == 0);
> > > -
> > > - return NULL;
> > > + } while (sctp_wait_for_packet(sk, &error, &timeo) == 0);
> > >
> > > no_packet:
> > > - *err = error;
> > > return NULL;
> > > }
> > >
> > > diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
> > > index 8920ca92a011..8ed51a15c3a4 100644
> > > --- a/net/sctp/ulpevent.c
> > > +++ b/net/sctp/ulpevent.c
> > > @@ -1061,9 +1061,8 @@ void sctp_ulpevent_read_nxtinfo(const struct sctp_ulpevent *event,
> > > struct sock *sk)
> > > {
> > > struct sk_buff *skb;
> > > - int err;
> > >
> > > - skb = sctp_skb_recv_datagram(sk, MSG_PEEK | MSG_DONTWAIT, &err);
> > > + skb = sctp_skb_recv_datagram(sk, MSG_PEEK | MSG_DONTWAIT);
> > > if (skb != NULL) {
> > > __sctp_ulpevent_read_nxtinfo(sctp_skb2event(skb),
> > > msghdr, skb);
> > > --
> > > 2.25.1
> > >
> > I think it's used at [1] in sctp_recvmsg():
> >
> > skb = sctp_skb_recv_datagram(sk, flags, &err);
> > if (!skb)
> > goto out;
>
> Would it make more sense to ERR_PTR() etc ?
>
Let's keep consistent with skb_recv_datagram() where it's &err,
besides, there's a shutdown condition (returns 0 via err param).
Thanks.
^ permalink raw reply
* Re: [PATCH RESEND v4 net-next 09/14] net: enetc: open-code enetc4_set_default_si_vlan_promisc()
From: Joe Damato @ 2026-07-20 15:26 UTC (permalink / raw)
To: wei.fang
Cc: claudiu.manoil, vladimir.oltean, xiaoning.wang, andrew+netdev,
davem, edumazet, kuba, pabeni, linux, wei.fang, chleroy,
maxime.chevallier, imx, netdev, linux-kernel, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20260720014317.1059359-10-wei.fang@oss.nxp.com>
On Mon, Jul 20, 2026 at 09:43:11AM +0800, wei.fang@oss.nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.com>
>
> The function enetc4_set_default_si_vlan_promisc() is only called once,
> from enetc4_configure_port_si(). Open-code the loop at the call site
> and remove the single-use wrapper.
>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
> drivers/net/ethernet/freescale/enetc/enetc4_pf.c | 15 +++------------
> 1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> index 859b02f5170a..505e4abf6c37 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> @@ -307,17 +307,6 @@ static void enetc4_pf_set_si_vlan_promisc(struct enetc_hw *hw, int si, bool en)
> enetc_port_wr(hw, ENETC4_PSIPVMR, val);
> }
>
> -static void enetc4_set_default_si_vlan_promisc(struct enetc_pf *pf)
> -{
> - struct enetc_hw *hw = &pf->si->hw;
> - int num_si = pf->caps.num_vsi + 1;
> - int i;
> -
> - /* enforce VLAN promiscuous mode for all SIs */
> - for (i = 0; i < num_si; i++)
> - enetc4_pf_set_si_vlan_promisc(hw, i, true);
> -}
> -
> /* Allocate the number of MSI-X vectors for per SI. */
> static void enetc4_set_si_msix_num(struct enetc_pf *pf)
> {
> @@ -361,7 +350,9 @@ static void enetc4_configure_port_si(struct enetc_pf *pf)
> /* Outer VLAN tag will be used for VLAN filtering */
> enetc_port_wr(hw, ENETC4_PSIVLANFMR, PSIVLANFMR_VS);
>
> - enetc4_set_default_si_vlan_promisc(pf);
> + /* Enforce VLAN promiscuous mode for all SIs */
> + for (int i = 0; i < pf->caps.num_vsi + 1; i++)
> + enetc4_pf_set_si_vlan_promisc(hw, i, true);
>
> /* Disable SI MAC multicast & unicast promiscuous */
> enetc_port_wr(hw, ENETC4_PSIPMMR, 0);
Reviewed-by: Joe Damato <joe@dama.to>
^ permalink raw reply
* Re: [PATCH RESEND v4 net-next 11/14] net: enetc: move enetc_set_si_vlan_promisc() to enetc_pf_common.c
From: Joe Damato @ 2026-07-20 15:28 UTC (permalink / raw)
To: wei.fang
Cc: claudiu.manoil, vladimir.oltean, xiaoning.wang, andrew+netdev,
davem, edumazet, kuba, pabeni, linux, wei.fang, chleroy,
maxime.chevallier, imx, netdev, linux-kernel, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20260720014317.1059359-12-wei.fang@oss.nxp.com>
On Mon, Jul 20, 2026 at 09:43:13AM +0800, wei.fang@oss.nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.com>
>
> The PSIPVMR in ENETC v4 has the same bit layout and functionality as the
> PSIPVMR register in ENETC v1: bit n (n <= 15) controls VLAN promiscuous
> mode for SI n. The only difference between the two hardware generations
> is the register address offset.
>
> Since the register functionality is identical, the VLAN promiscuous mode
> setting code can be shared between ENETC v1 and v4 drivers.
>
> Move enetc_set_si_vlan_promisc() from enetc_pf.c to enetc_pf_common.c
> and export it so that it can be shared between the two drivers. Add a
> revision check using is_enetc_rev1() to select the correct register
> offset (ENETC_PSIPVMR for v1 and ENETC4_PSIPVMR for v4) while keeping
> the same logic.
>
> Remove the v4-specific enetc4_pf_set_si_vlan_promisc() from enetc4_pf.c
> and replace its call site with the new common enetc_set_si_vlan_promisc()
> to eliminate code duplication.
>
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
> .../net/ethernet/freescale/enetc/enetc4_pf.c | 17 ++------------
> .../net/ethernet/freescale/enetc/enetc_pf.c | 16 --------------
> .../freescale/enetc/enetc_pf_common.c | 22 +++++++++++++++++++
> .../freescale/enetc/enetc_pf_common.h | 1 +
> 4 files changed, 25 insertions(+), 31 deletions(-)
Reviewed-by: Joe Damato <joe@dama.to>
^ permalink raw reply
* Re: [PATCH net-next] net: stmmac: Simplify ioctl handling
From: Maxime Chevallier @ 2026-07-20 15:34 UTC (permalink / raw)
To: Vadim Fedorenko, Andrew Lunn
Cc: Andrew Lunn, Jakub Kicinski, davem, Eric Dumazet, Paolo Abeni,
Simon Horman, Maxime Coquelin, Alexandre Torgue, Russell King,
thomas.petazzoni, Alexis Lothoré, netdev, linux-kernel,
linux-arm-kernel, linux-stm32
In-Reply-To: <6ac12388-60de-45aa-a8d0-62fcfaf7bea2@linux.dev>
On 7/20/26 17:17, Vadim Fedorenko wrote:
> On 19.07.2026 17:13, Andrew Lunn wrote:
>>> Looking at this, I'm wondering if we can't just get rid of SIOCSHWTSTAMP
>>> handling in phy_mii_ioctl(). Looks like we can ?
>>
>> I'm not sure about that. We need Richards input.
>>
>> The code in phy_mii_ioctl() allows the MAC to be bypassed, it goes
>> straight to a PHY based stamper. It could be the MAC has no idea the
>> PHY has this capability, so it has not implemented the .ndo?
Indeed, but even then the SIOCxHWTSTAMP aren't reaching the ndo_ioctl.
Maybe the thing to change (unrelated to the icotl though) is in
dev_set_hwtstamp :
if (!ops->ndo_hwtstamp_set)
return -EOPNOTSUPP;
if (!netif_device_present(dev))
return -ENODEV;
netdev_lock_ops(dev);
err = dev_set_hwtstamp_phylib(dev, &kernel_cfg, &extack);
netdev_unlock_ops(dev);
We don't try to configure the PHY timestamping if the MAC doesn't support the
.ndo, maybe we should allow that ?
>>
>> It might be we need to hoist the code from phy_mii_ioctl() into
>> dev_{sg}et_hwtstamp()?
>
> Hi Andrew!
>
> I think I've converted all phy drivers while removing support for
> SIOCSHWTSTAMP/SIOCGHWTSTAMP from netdev ioctl. I believe it's impossible right
> now to reach SIOCSHWTSTAMP path of phy_mii_ioctl via ioctl on net device.
That was also my understanding indeed.
> Is it possible to have ioctl on phy device directly without involving netdev?
Unless there's an obscure mechanism I don't know about, there shouldn't be a way.
SIOCSHWTSTAMP/SIOCGHWTSTAMP don't seem to reach netdev anymore with the (great !) work
you've done, so they won't reach the PHY either indeed.
The SIOC ioctls can't reach PHYs without a netdev, neither can the ethnl ones.
Even when we have multiple PHYs and we use netlink, we can only reach the ones
behind a netdevice.
There is still sysfs entries for standalone PHYs, but I don't think we can use
any of that for ioctl.
Maxime
^ permalink raw reply
* Re: [PATCH net-next v9 12/12] net: airoha: add phylink support
From: Lorenzo Bianconi @ 2026-07-20 15:35 UTC (permalink / raw)
To: Christian Marangi
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Simon Horman, Jonathan Corbet, Shuah Khan, Heiner Kallweit,
Russell King, Saravana Kannan, Philipp Zabel, netdev, devicetree,
linux-kernel, linux-doc, linux-arm-kernel, linux-mediatek,
Maxime Chevallier
In-Reply-To: <20260717065448.1498335-13-ansuelsmth@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 12669 bytes --]
> Add phylink support for each GDM port. For GDM1 add the internal interface
> mode as the only supported mode. For GDM2/3/4 add the required
> configuration of the PCS to make the external PHY or attached SFP cage
> work.
>
> These needs to be defined in the GDM port node using the pcs-handle
> property.
>
> Update and provide a .get/set_link_ksettings function that use phylink
> for ethtool OPs now that we fully support phylink.
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> drivers/net/ethernet/airoha/Kconfig | 1 +
> drivers/net/ethernet/airoha/airoha_eth.c | 194 +++++++++++++++++++++-
> drivers/net/ethernet/airoha/airoha_eth.h | 7 +-
> drivers/net/ethernet/airoha/airoha_regs.h | 12 ++
> 4 files changed, 207 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/airoha/Kconfig b/drivers/net/ethernet/airoha/Kconfig
> index 1f6640a15fc9..789906516bf8 100644
> --- a/drivers/net/ethernet/airoha/Kconfig
> +++ b/drivers/net/ethernet/airoha/Kconfig
> @@ -20,6 +20,7 @@ config NET_AIROHA
> depends on NET_DSA || !NET_DSA
> select NET_AIROHA_NPU
> select PAGE_POOL
> + select PHYLINK
> help
> This driver supports the gigabit ethernet MACs in the
> Airoha SoC family.
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 59001fd4b6f7..ed1ac032f337 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -8,6 +8,7 @@
> #include <linux/of_reserved_mem.h>
> #include <linux/platform_device.h>
> #include <linux/tcp.h>
> +#include <linux/pcs/pcs.h>
> #include <linux/u64_stats_sync.h>
> #include <net/dst_metadata.h>
> #include <net/page_pool/helpers.h>
> @@ -1837,7 +1838,7 @@ static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
> struct airoha_gdm_port *port = dev->port;
> int i;
>
> - spin_lock(&port->stats_lock);
> + spin_lock(&port->lock);
Hi Christian,
as pointed out in a previous email, I do not like the approach of reusing this
spin_lock for airoha_mac_link_up(). Can we use rtl_lock() (when necessary) as
pointed out before?
Regards,
Lorenzo
>
> for (i = 0; i < ARRAY_SIZE(port->devs); i++) {
> if (port->devs[i])
> @@ -1848,7 +1849,7 @@ static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
> airoha_fe_set(dev->eth, REG_FE_GDM_MIB_CLEAR(port->id),
> FE_GDM_MIB_RX_CLEAR_MASK | FE_GDM_MIB_TX_CLEAR_MASK);
>
> - spin_unlock(&port->stats_lock);
> + spin_unlock(&port->lock);
> }
>
> static void airoha_dev_set_xmit_frame_size(struct net_device *netdev)
> @@ -1870,6 +1871,14 @@ static int airoha_dev_open(struct net_device *netdev)
> u32 pse_port = FE_PSE_PORT_PPE1;
> int err;
>
> + err = phylink_of_phy_connect(dev->phylink, netdev->dev.of_node, 0);
> + if (err) {
> + netdev_err(netdev, "could not attach PHY: %d\n", err);
> + return err;
> + }
> +
> + phylink_start(dev->phylink);
> +
> netif_tx_start_all_queues(netdev);
> err = airoha_set_vip_for_gdm_port(dev, true);
> if (err)
> @@ -1909,6 +1918,10 @@ static int airoha_dev_stop(struct net_device *netdev)
> airoha_set_gdm_port_fwd_cfg(qdma->eth,
> REG_GDM_FWD_CFG(port->id),
> FE_PSE_PORT_DROP);
> +
> + phylink_stop(dev->phylink);
> + phylink_disconnect_phy(dev->phylink);
> +
> return 0;
> }
>
> @@ -2389,6 +2402,24 @@ airoha_ethtool_get_rmon_stats(struct net_device *netdev,
> } while (u64_stats_fetch_retry(&dev->stats.syncp, start));
> }
>
> +static int
> +airoha_ethtool_get_link_ksettings(struct net_device *netdev,
> + struct ethtool_link_ksettings *cmd)
> +{
> + struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +
> + return phylink_ethtool_ksettings_get(dev->phylink, cmd);
> +}
> +
> +static int
> +airoha_ethtool_set_link_ksettings(struct net_device *netdev,
> + const struct ethtool_link_ksettings *cmd)
> +{
> + struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +
> + return phylink_ethtool_ksettings_set(dev->phylink, cmd);
> +}
> +
> static int airoha_qdma_set_chan_tx_sched(struct net_device *netdev,
> int channel, enum tx_sched_mode mode,
> const u16 *weights, u8 n_weights)
> @@ -3120,7 +3151,8 @@ static const struct ethtool_ops airoha_ethtool_ops = {
> .get_drvinfo = airoha_ethtool_get_drvinfo,
> .get_eth_mac_stats = airoha_ethtool_get_mac_stats,
> .get_rmon_stats = airoha_ethtool_get_rmon_stats,
> - .get_link_ksettings = phy_ethtool_get_link_ksettings,
> + .get_link_ksettings = airoha_ethtool_get_link_ksettings,
> + .set_link_ksettings = airoha_ethtool_set_link_ksettings,
> .get_link = ethtool_op_get_link,
> };
>
> @@ -3176,6 +3208,155 @@ bool airoha_is_valid_gdm_dev(struct airoha_eth *eth,
> return false;
> }
>
> +/* Nothing to do in MAC, everything is handled in PCS */
> +static void airoha_mac_config(struct phylink_config *config, unsigned int mode,
> + const struct phylink_link_state *state)
> +{
> +}
> +
> +static void airoha_mac_link_up(struct phylink_config *config, struct phy_device *phy,
> + unsigned int mode, phy_interface_t interface,
> + int speed, int duplex, bool tx_pause, bool rx_pause)
> +{
> + struct airoha_gdm_dev *dev = container_of(config, struct airoha_gdm_dev,
> + phylink_config);
> + struct airoha_gdm_port *port = dev->port;
> + struct airoha_eth *eth = dev->eth;
> + u32 frag_size_tx, frag_size_rx;
> + u32 mask, val;
> +
> + /* TX/RX frag is configured only for GDM4 */
> + if (port->id != AIROHA_GDM4_IDX)
> + return;
> +
> + switch (speed) {
> + case SPEED_10000:
> + case SPEED_5000:
> + frag_size_tx = 8;
> + frag_size_rx = 8;
> + break;
> + case SPEED_2500:
> + frag_size_tx = 2;
> + frag_size_rx = 1;
> + break;
> + default:
> + frag_size_tx = 1;
> + frag_size_rx = 0;
> + }
> +
> + spin_lock(&port->lock);
> +
> + /* Configure TX/RX frag based on speed */
> + if (dev->nbq == 1) {
> + mask = GDM4_SGMII1_TX_FRAG_SIZE_MASK;
> + val = FIELD_PREP(GDM4_SGMII1_TX_FRAG_SIZE_MASK,
> + frag_size_tx);
> + } else {
> + mask = GDM4_SGMII0_TX_FRAG_SIZE_MASK;
> + val = FIELD_PREP(GDM4_SGMII0_TX_FRAG_SIZE_MASK,
> + frag_size_tx);
> + }
> + airoha_fe_rmw(eth, REG_FE_GDM4_TMBI_FRAG, mask, val);
> +
> + if (dev->nbq == 1) {
> + mask = GDM4_SGMII1_RX_FRAG_SIZE_MASK;
> + val = FIELD_PREP(GDM4_SGMII1_RX_FRAG_SIZE_MASK,
> + frag_size_rx);
> + } else {
> + mask = GDM4_SGMII0_RX_FRAG_SIZE_MASK;
> + val = FIELD_PREP(GDM4_SGMII0_RX_FRAG_SIZE_MASK,
> + frag_size_rx);
> + }
> + airoha_fe_rmw(eth, REG_FE_GDM4_RMBI_FRAG, mask, val);
> +
> + spin_unlock(&port->lock);
> +}
> +
> +/* Nothing to do in MAC, everything is handled in PCS */
> +static void airoha_mac_link_down(struct phylink_config *config, unsigned int mode,
> + phy_interface_t interface)
> +{
> +}
> +
> +static const struct phylink_mac_ops airoha_phylink_ops = {
> + .mac_config = airoha_mac_config,
> + .mac_link_up = airoha_mac_link_up,
> + .mac_link_down = airoha_mac_link_down,
> +};
> +
> +static int airoha_fill_available_pcs(struct phylink_config *config,
> + struct phylink_pcs **available_pcs,
> + unsigned int num_possible_pcs)
> +{
> + struct device *dev = config->dev;
> +
> + return fwnode_phylink_pcs_parse(dev_fwnode(dev), available_pcs,
> + num_possible_pcs);
> +}
> +
> +static int airoha_setup_phylink(struct net_device *netdev)
> +{
> + struct airoha_gdm_dev *dev = netdev_priv(netdev);
> + struct device_node *np = netdev->dev.of_node;
> + struct airoha_gdm_port *port = dev->port;
> + struct phylink_config *config;
> + phy_interface_t phy_mode;
> + struct phylink *phylink;
> + int err;
> +
> + err = of_get_phy_mode(np, &phy_mode);
> + if (err) {
> + dev_err(&netdev->dev, "incorrect phy-mode\n");
> + return err;
> + }
> +
> + config = &dev->phylink_config;
> + config->dev = &netdev->dev;
> + config->type = PHYLINK_NETDEV;
> +
> + /*
> + * GDM1 only supports internal for Embedded Switch
> + * and doesn't require a PCS.
> + */
> + if (port->id == AIROHA_GDM1_IDX) {
> + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> + MAC_10000FD;
> +
> + __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> + config->supported_interfaces);
> + } else {
> + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> + MAC_10 | MAC_100 | MAC_1000 |
> + MAC_2500FD | MAC_5000FD | MAC_10000FD;
> +
> + config->num_possible_pcs = fwnode_phylink_pcs_count(dev_fwnode(config->dev));
> + config->fill_available_pcs = airoha_fill_available_pcs;
> +
> + __set_bit(PHY_INTERFACE_MODE_SGMII,
> + config->supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_1000BASEX,
> + config->supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_2500BASEX,
> + config->supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_10GBASER,
> + config->supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_USXGMII,
> + config->supported_interfaces);
> +
> + phy_interface_copy(config->pcs_interfaces,
> + config->supported_interfaces);
> + }
> +
> + phylink = phylink_create(config, of_fwnode_handle(np),
> + phy_mode, &airoha_phylink_ops);
> + if (IS_ERR(phylink))
> + return PTR_ERR(phylink);
> +
> + dev->phylink = phylink;
> +
> + return 0;
> +}
> +
> static int airoha_alloc_gdm_device(struct airoha_eth *eth,
> struct airoha_gdm_port *port,
> int nbq, struct device_node *np)
> @@ -3239,7 +3420,7 @@ static int airoha_alloc_gdm_device(struct airoha_eth *eth,
> dev->nbq = nbq;
> port->devs[index] = dev;
>
> - return 0;
> + return airoha_setup_phylink(netdev);
> }
>
> static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> @@ -3274,7 +3455,7 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> return -ENOMEM;
>
> port->id = id;
> - spin_lock_init(&port->stats_lock);
> + spin_lock_init(&port->lock);
> eth->ports[p] = port;
>
> err = airoha_metadata_dst_alloc(port);
> @@ -3471,6 +3652,8 @@ static int airoha_probe(struct platform_device *pdev)
> netdev = netdev_from_priv(dev);
> if (netdev->reg_state == NETREG_REGISTERED)
> unregister_netdev(netdev);
> + if (dev->phylink)
> + phylink_destroy(dev->phylink);
> of_node_put(netdev->dev.of_node);
> }
> airoha_metadata_dst_free(port);
> @@ -3509,6 +3692,7 @@ static void airoha_remove(struct platform_device *pdev)
>
> netdev = netdev_from_priv(dev);
> unregister_netdev(netdev);
> + phylink_destroy(dev->phylink);
> of_node_put(netdev->dev.of_node);
> }
> airoha_metadata_dst_free(port);
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index f6d01a8e8da1..b49fc5304b3a 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -561,6 +561,9 @@ struct airoha_gdm_dev {
> int nbq;
>
> struct airoha_hw_stats stats;
> +
> + struct phylink *phylink;
> + struct phylink_config phylink_config;
> };
>
> struct airoha_gdm_port {
> @@ -568,8 +571,8 @@ struct airoha_gdm_port {
> int id;
> int users;
>
> - /* protect concurrent hw_stats accesses */
> - spinlock_t stats_lock;
> + /* protect concurrent hw_stats and frag register accesses */
> + spinlock_t lock;
>
> struct metadata_dst *dsa_meta[AIROHA_MAX_DSA_PORTS];
> };
> diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
> index 6fed63d013b4..8df02f51211c 100644
> --- a/drivers/net/ethernet/airoha/airoha_regs.h
> +++ b/drivers/net/ethernet/airoha/airoha_regs.h
> @@ -357,6 +357,18 @@
> #define IP_FRAGMENT_PORT_MASK GENMASK(8, 5)
> #define IP_FRAGMENT_NBQ_MASK GENMASK(4, 0)
>
> +#define REG_FE_GDM4_TMBI_FRAG 0x2028
> +#define GDM4_SGMII1_TX_WEIGHT_MASK GENMASK(31, 26)
> +#define GDM4_SGMII1_TX_FRAG_SIZE_MASK GENMASK(25, 16)
> +#define GDM4_SGMII0_TX_WEIGHT_MASK GENMASK(15, 10)
> +#define GDM4_SGMII0_TX_FRAG_SIZE_MASK GENMASK(9, 0)
> +
> +#define REG_FE_GDM4_RMBI_FRAG 0x202c
> +#define GDM4_SGMII1_RX_WEIGHT_MASK GENMASK(31, 26)
> +#define GDM4_SGMII1_RX_FRAG_SIZE_MASK GENMASK(25, 16)
> +#define GDM4_SGMII0_RX_WEIGHT_MASK GENMASK(15, 10)
> +#define GDM4_SGMII0_RX_FRAG_SIZE_MASK GENMASK(9, 0)
> +
> #define REG_MC_VLAN_EN 0x2100
> #define MC_VLAN_EN_MASK BIT(0)
>
> --
> 2.53.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* RE: [PATCH net v8 1/3] tipc: fix NULL deref in tipc_named_node_up() on empty publication list
From: Tung Quang Nguyen @ 2026-07-20 15:41 UTC (permalink / raw)
To: Weiming Shi
Cc: Xiang Mei, kernel test robot, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman,
linux-kernel@vger.kernel.org, Jon Maloy, netdev@vger.kernel.org,
tipc-discussion@lists.sourceforge.net
In-Reply-To: <20260718092544.785289-2-bestswngs@gmail.com>
>Subject: [PATCH net v8 1/3] tipc: fix NULL deref in tipc_named_node_up() on
>empty publication list
>
>[The defer-to-workqueue approach is by Tung Nguyen. He posted it on the
>thread and asked us to test it, as the replacement for the item-less bulk
>approach. Since the RFC only exists as an inline diff in the thread, it is folded
>into this series so the fix is self-contained.]
>
>named_distribute() stamps the last_bulk flag on the tail skb of the publication
>list. When the list is empty no skb is enqueued and the tail access dereferences
>NULL. tipc_named_node_up() hits this on an empty cluster_scope, which
>happens with a node-id configuration where cluster_scope is populated only
>later by tipc_net_finalize(). It is reachable by an unprivileged user over a UDP
>bearer in a user+net namespace. The reported crash:
>
> KASAN: null-ptr-deref in range [0x00000000000000d8-0x00000000000000df]
> RIP: 0010:tipc_named_node_up (net/tipc/name_distr.c:196)
> tipc_named_node_up (net/tipc/name_distr.c:196 net/tipc/name_distr.c:221)
> tipc_node_write_unlock (net/tipc/node.c:428)
> tipc_rcv (net/tipc/node.c:2185)
> tipc_udp_recv (net/tipc/udp_media.c:392) Kernel panic - not syncing: Fatal
>exception in interrupt
>
>When cluster_scope is empty at node-up, defer the bulk distribution to a
>workqueue and wait for tipc_net_finalize() to publish the node-state name, so
>named_distribute() always runs on a non-empty list. On allocation failure,
>purge the partially built queue and bring the link down so the bulk distribution
>restarts when the link comes up again.
>
>Fixes: cad2929dc432 ("tipc: update a binding service via broadcast")
>Reported-by: Xiang Mei <xmei5@asu.edu>
>Reported-by: kernel test robot <lkp@intel.com>
>Closes: https://lore.kernel.org/oe-kbuild-all/202607180730.TwVgASDI-
>lkp@intel.com/
>Signed-off-by: Tung Nguyen <tung.quang.nguyen@est.tech>
>Tested-by: Weiming Shi <bestswngs@gmail.com>
>Signed-off-by: Weiming Shi <bestswngs@gmail.com>
>---
sashiko reports many critical/high issues: https://sashiko.dev/#/patchset/20260718092544.785289-1-bestswngs%40gmail.com
I address all in below patch. Could you please test it ?
---
net/tipc/core.c | 2 ++
net/tipc/core.h | 4 +++
net/tipc/name_distr.c | 67 +++++++++++++++++++++++++++++++++++++------
net/tipc/name_distr.h | 3 +-
net/tipc/net.c | 15 +++++++++-
net/tipc/node.c | 61 +++++++++++++++++++++++++++++++++++++--
6 files changed, 139 insertions(+), 13 deletions(-)
diff --git a/net/tipc/core.c b/net/tipc/core.c
index 315975c3be81..52544b805dcc 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -61,6 +61,8 @@ static int __net_init tipc_init_net(struct net *net)
tn->trial_addr = 0;
tn->addr_trial_end = 0;
tn->capabilities = TIPC_NODE_CAPABILITIES;
+ atomic_set(&tn->finalized, 0);
+ atomic_set(&tn->work_rescheduled, 0);
INIT_WORK(&tn->work, tipc_net_finalize_work);
memset(tn->node_id, 0, sizeof(tn->node_id));
memset(tn->node_id_string, 0, sizeof(tn->node_id_string));
diff --git a/net/tipc/core.h b/net/tipc/core.h
index 9ce5f9ff6cc0..52fdb9189cc3 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -145,6 +145,10 @@ struct tipc_net {
struct work_struct work;
/* The numbers of work queues in schedule */
atomic_t wq_count;
+ /* flag to indicate work has finished */
+ atomic_t finalized;
+ /* flag to reschedule work */
+ atomic_t work_rescheduled;
};
static inline struct tipc_net *tipc_net(struct net *net)
diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index ba4f4906e13b..3e32445cfbd0 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -147,8 +147,8 @@ struct sk_buff *tipc_named_withdraw(struct net *net, struct publication *p)
* @pls: linked list of publication items to be packed into buffer chain
* @seqno: sequence number for this message
*/
-static void named_distribute(struct net *net, struct sk_buff_head *list,
- u32 dnode, struct list_head *pls, u16 seqno)
+static int named_distribute(struct net *net, struct sk_buff_head *list,
+ u32 dnode, struct list_head *pls, u16 seqno)
{
struct publication *publ;
struct sk_buff *skb = NULL;
@@ -164,8 +164,9 @@ static void named_distribute(struct net *net, struct sk_buff_head *list,
skb = named_prepare_buf(net, PUBLICATION, msg_rem,
dnode);
if (!skb) {
+ __skb_queue_purge(list);
pr_warn("Bulk publication failure\n");
- return;
+ return 1;
}
hdr = buf_msg(skb);
msg_set_bc_ack_invalid(hdr, true);
@@ -195,15 +196,16 @@ static void named_distribute(struct net *net, struct sk_buff_head *list,
hdr = buf_msg(skb_peek_tail(list));
msg_set_last_bulk(hdr);
msg_set_named_seqno(hdr, seqno);
+
+ return 0;
}
/**
- * tipc_named_node_up - tell specified node about all publications by this node
+ * tipc_named_distribute - distribute all publications to specified node
* @net: the associated network namespace
* @dnode: destination node
- * @capabilities: peer node's capabilities
*/
-void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
+static int tipc_named_distribute(struct net *net, u32 dnode)
{
struct name_table *nt = tipc_name_table(net);
struct tipc_net *tn = tipc_net(net);
@@ -212,15 +214,62 @@ void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
__skb_queue_head_init(&head);
spin_lock_bh(&tn->nametbl_lock);
- if (!(capabilities & TIPC_NAMED_BCAST))
- nt->rc_dests++;
seqno = nt->snd_nxt;
spin_unlock_bh(&tn->nametbl_lock);
read_lock_bh(&nt->cluster_scope_lock);
- named_distribute(net, &head, dnode, &nt->cluster_scope, seqno);
+ /* 1. tipc_net_finalize_work() is not scheduled because of namespace
+ * teardown.
+ * 2. Or tipc_net_finalize() ---> tipc_nametbl_publish() has failed
+ * to insert node self address publication to nt->cluster_scope.
+ * 3. Or tipc_net_finalize() ---> tipc_nametbl_publish() has not
+ * executed yet.
+ */
+ if (unlikely(list_empty(&nt->cluster_scope))) {
+ read_unlock_bh(&nt->cluster_scope_lock);
+ return 1;
+ }
+
+ if (named_distribute(net, &head, dnode, &nt->cluster_scope, seqno)) {
+ read_unlock_bh(&nt->cluster_scope_lock);
+ return -ENOBUFS;
+ }
tipc_node_xmit(net, &head, dnode, 0);
read_unlock_bh(&nt->cluster_scope_lock);
+
+ return 0;
+}
+
+/**
+ * tipc_named_node_up - tell specified node about all publications by this node
+ * @net: the associated network namespace
+ * @dnode: destination node
+ * @capabilities: peer node's capabilities
+ */
+int tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
+{
+ struct name_table *nt = tipc_name_table(net);
+ struct tipc_net *tn = tipc_net(net);
+
+ spin_lock_bh(&tn->nametbl_lock);
+ if (!(capabilities & TIPC_NAMED_BCAST))
+ nt->rc_dests++;
+ spin_unlock_bh(&tn->nametbl_lock);
+
+ return tipc_named_distribute(net, dnode);
+}
+
+/**
+ * tipc_named_dist_cluster_scope - distribute all publications to specified node
+ * @net: the associated network namespace
+ * @dnode: destination node
+ */
+int tipc_named_dist_cluster_scope(struct net *net, u32 dnode)
+{
+ struct tipc_net *tn = tipc_net(net);
+
+ wait_var_event(&tn->finalized, atomic_read(&tn->finalized));
+ return tipc_named_distribute(net, dnode);
}
/**
diff --git a/net/tipc/name_distr.h b/net/tipc/name_distr.h
index c677f6f082df..cadf4e8c3e66 100644
--- a/net/tipc/name_distr.h
+++ b/net/tipc/name_distr.h
@@ -69,7 +69,8 @@ struct distr_item {
struct sk_buff *tipc_named_publish(struct net *net, struct publication *publ);
struct sk_buff *tipc_named_withdraw(struct net *net, struct publication *publ);
-void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities);
+int tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities);
+int tipc_named_dist_cluster_scope(struct net *net, u32 dnode);
void tipc_named_rcv(struct net *net, struct sk_buff_head *namedq,
u16 *rcv_nxt, bool *open);
void tipc_named_reinit(struct net *net);
diff --git a/net/tipc/net.c b/net/tipc/net.c
index 7e65d0b0c4a8..78418515277e 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -132,13 +132,26 @@ static void tipc_net_finalize(struct net *net, u32 addr)
tipc_uaddr(&ua, TIPC_SERVICE_RANGE, TIPC_CLUSTER_SCOPE,
TIPC_NODE_STATE, addr, addr);
+ if (atomic_read(&tn->work_rescheduled))
+ goto publish;
+
if (cmpxchg(&tn->node_addr, 0, addr))
return;
tipc_set_node_addr(net, addr);
tipc_named_reinit(net);
tipc_sk_reinit(net);
tipc_mon_reinit_self(net);
- tipc_nametbl_publish(net, &ua, &sk, addr);
+
+publish:
+ if (!tipc_nametbl_publish(net, &ua, &sk, addr)) {
+ tn->trial_addr = addr;
+ atomic_set(&tn->work_rescheduled, 1);
+ schedule_work(&tn->work);
+ return;
+ }
+ atomic_set(&tn->work_rescheduled, 0);
+ atomic_set(&tn->finalized, 1);
+ wake_up_var(&tn->finalized);
}
void tipc_net_finalize_work(struct work_struct *work)
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 8e4ef2630ae4..afed72894722 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -111,6 +111,8 @@ struct tipc_bclink_entry {
* @peer_net: peer's net namespace
* @peer_hash_mix: hash for this peer (FIXME)
* @crypto_rx: RX crypto handler
+ * @work: work item for bulk distribution of cluster scope publications
+ * @work_scheduled: flag to indicate the work has been scheduled
*/
struct tipc_node {
u32 addr;
@@ -145,6 +147,8 @@ struct tipc_node {
#ifdef CONFIG_TIPC_CRYPTO
struct tipc_crypto *crypto_rx;
#endif
+ struct work_struct work;
+ atomic_t work_scheduled;
};
/* Node FSM states and events:
@@ -393,6 +397,27 @@ static void tipc_node_write_unlock_fast(struct tipc_node *n)
write_unlock_bh(&n->lock);
}
+static void tipc_node_down(struct tipc_node *n)
+{
+ u32 bearer_id, bearer_cnt;
+
+ tipc_node_read_lock(n);
+ bearer_cnt = n->link_cnt;
+ tipc_node_read_unlock(n);
+ for (bearer_id = 0; bearer_id < bearer_cnt; bearer_id++)
+ tipc_node_link_down(n, bearer_id, false);
+}
+
+static void tipc_node_dist_bulk(struct work_struct *work)
+{
+ struct tipc_node *node = container_of(work, struct tipc_node, work);
+
+ if (tipc_named_dist_cluster_scope(node->net, node->addr) < 0)
+ tipc_node_down(node);
+
+ tipc_node_put(node);
+}
+
static void tipc_node_write_unlock(struct tipc_node *n)
__releases(n->lock)
{
@@ -424,8 +449,23 @@ static void tipc_node_write_unlock(struct tipc_node *n)
if (flags & TIPC_NOTIFY_NODE_DOWN)
tipc_publ_notify(net, publ_list, node, n->capabilities);
- if (flags & TIPC_NOTIFY_NODE_UP)
- tipc_named_node_up(net, node, n->capabilities);
+ if (flags & TIPC_NOTIFY_NODE_UP) {
+ int rc = 0;
+
+ rc = tipc_named_node_up(net, node, n->capabilities);
+ /* Defer bulk distribution to work queue */
+ if (rc > 0) {
+ atomic_set(&n->work_scheduled, 1);
+ tipc_node_get(n);
+ if (!schedule_work(&n->work))
+ tipc_node_put(n);
+ } else if (rc < 0) {
+ /* Bring the node down to start over bulk distribution
+ * when the first link is up again.
+ */
+ tipc_node_down(n);
+ }
+ }
if (flags & TIPC_NOTIFY_LINK_UP) {
tipc_mon_peer_up(net, node, bearer_id);
@@ -564,6 +604,8 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
INIT_LIST_HEAD(&n->list);
INIT_LIST_HEAD(&n->publ_list);
INIT_LIST_HEAD(&n->conn_sks);
+ INIT_WORK(&n->work, tipc_node_dist_bulk);
+ atomic_set(&n->work_scheduled, 0);
skb_queue_head_init(&n->bc_entry.namedq);
skb_queue_head_init(&n->bc_entry.inputq1);
__skb_queue_head_init(&n->bc_entry.arrvq);
@@ -635,10 +677,25 @@ static void tipc_node_delete_from_list(struct tipc_node *node)
static void tipc_node_delete(struct tipc_node *node)
{
+ struct tipc_net *tn = tipc_net(node->net);
+
trace_tipc_node_delete(node, true, " ");
tipc_node_delete_from_list(node);
timer_delete_sync(&node->timer);
+
+ /* Wake up node work queue if tipc_net_finalize_work() is not
+ * scheduled yet.
+ */
+ if (atomic_read(&node->work_scheduled)) {
+ if (!atomic_read(&tn->finalized)) {
+ atomic_set(&tn->finalized, 1);
+ wake_up_var(&tn->finalized);
+ }
+
+ cancel_work_sync(&node->work);
+ }
+
tipc_node_put(node);
}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH net-next v9 12/12] net: airoha: add phylink support
From: Christian Marangi @ 2026-07-20 15:43 UTC (permalink / raw)
To: Lorenzo Bianconi
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Simon Horman, Jonathan Corbet, Shuah Khan, Heiner Kallweit,
Russell King, Saravana Kannan, Philipp Zabel, netdev, devicetree,
linux-kernel, linux-doc, linux-arm-kernel, linux-mediatek,
Maxime Chevallier
In-Reply-To: <al5AVLxWRhnpGzJ1@lore-desk>
On Mon, Jul 20, 2026 at 05:35:48PM +0200, Lorenzo Bianconi wrote:
> > Add phylink support for each GDM port. For GDM1 add the internal interface
> > mode as the only supported mode. For GDM2/3/4 add the required
> > configuration of the PCS to make the external PHY or attached SFP cage
> > work.
> >
> > These needs to be defined in the GDM port node using the pcs-handle
> > property.
> >
> > Update and provide a .get/set_link_ksettings function that use phylink
> > for ethtool OPs now that we fully support phylink.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> > drivers/net/ethernet/airoha/Kconfig | 1 +
> > drivers/net/ethernet/airoha/airoha_eth.c | 194 +++++++++++++++++++++-
> > drivers/net/ethernet/airoha/airoha_eth.h | 7 +-
> > drivers/net/ethernet/airoha/airoha_regs.h | 12 ++
> > 4 files changed, 207 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/airoha/Kconfig b/drivers/net/ethernet/airoha/Kconfig
> > index 1f6640a15fc9..789906516bf8 100644
> > --- a/drivers/net/ethernet/airoha/Kconfig
> > +++ b/drivers/net/ethernet/airoha/Kconfig
> > @@ -20,6 +20,7 @@ config NET_AIROHA
> > depends on NET_DSA || !NET_DSA
> > select NET_AIROHA_NPU
> > select PAGE_POOL
> > + select PHYLINK
> > help
> > This driver supports the gigabit ethernet MACs in the
> > Airoha SoC family.
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 59001fd4b6f7..ed1ac032f337 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -8,6 +8,7 @@
> > #include <linux/of_reserved_mem.h>
> > #include <linux/platform_device.h>
> > #include <linux/tcp.h>
> > +#include <linux/pcs/pcs.h>
> > #include <linux/u64_stats_sync.h>
> > #include <net/dst_metadata.h>
> > #include <net/page_pool/helpers.h>
> > @@ -1837,7 +1838,7 @@ static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
> > struct airoha_gdm_port *port = dev->port;
> > int i;
> >
> > - spin_lock(&port->stats_lock);
> > + spin_lock(&port->lock);
>
> Hi Christian,
>
> as pointed out in a previous email, I do not like the approach of reusing this
> spin_lock for airoha_mac_link_up(). Can we use rtl_lock() (when necessary) as
> pointed out before?
>
For context, quoting from the previous series, the suggestion is to use
rtnl_is_locked() and then lock accordingly but I didn't find other usage of
that in other driver (aside from core net) and I don't like the use of
is_locked. I can already see the BOT saying that in the timeframe of
is_locked and writing the register another interface goes up causing a
race.
Guess I will add a simple mutex for this case and the other.
> >
> > for (i = 0; i < ARRAY_SIZE(port->devs); i++) {
> > if (port->devs[i])
> > @@ -1848,7 +1849,7 @@ static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
> > airoha_fe_set(dev->eth, REG_FE_GDM_MIB_CLEAR(port->id),
> > FE_GDM_MIB_RX_CLEAR_MASK | FE_GDM_MIB_TX_CLEAR_MASK);
> >
> > - spin_unlock(&port->stats_lock);
> > + spin_unlock(&port->lock);
> > }
> >
> > static void airoha_dev_set_xmit_frame_size(struct net_device *netdev)
> > @@ -1870,6 +1871,14 @@ static int airoha_dev_open(struct net_device *netdev)
> > u32 pse_port = FE_PSE_PORT_PPE1;
> > int err;
> >
> > + err = phylink_of_phy_connect(dev->phylink, netdev->dev.of_node, 0);
> > + if (err) {
> > + netdev_err(netdev, "could not attach PHY: %d\n", err);
> > + return err;
> > + }
> > +
> > + phylink_start(dev->phylink);
> > +
> > netif_tx_start_all_queues(netdev);
> > err = airoha_set_vip_for_gdm_port(dev, true);
> > if (err)
> > @@ -1909,6 +1918,10 @@ static int airoha_dev_stop(struct net_device *netdev)
> > airoha_set_gdm_port_fwd_cfg(qdma->eth,
> > REG_GDM_FWD_CFG(port->id),
> > FE_PSE_PORT_DROP);
> > +
> > + phylink_stop(dev->phylink);
> > + phylink_disconnect_phy(dev->phylink);
> > +
> > return 0;
> > }
> >
> > @@ -2389,6 +2402,24 @@ airoha_ethtool_get_rmon_stats(struct net_device *netdev,
> > } while (u64_stats_fetch_retry(&dev->stats.syncp, start));
> > }
> >
> > +static int
> > +airoha_ethtool_get_link_ksettings(struct net_device *netdev,
> > + struct ethtool_link_ksettings *cmd)
> > +{
> > + struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > +
> > + return phylink_ethtool_ksettings_get(dev->phylink, cmd);
> > +}
> > +
> > +static int
> > +airoha_ethtool_set_link_ksettings(struct net_device *netdev,
> > + const struct ethtool_link_ksettings *cmd)
> > +{
> > + struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > +
> > + return phylink_ethtool_ksettings_set(dev->phylink, cmd);
> > +}
> > +
> > static int airoha_qdma_set_chan_tx_sched(struct net_device *netdev,
> > int channel, enum tx_sched_mode mode,
> > const u16 *weights, u8 n_weights)
> > @@ -3120,7 +3151,8 @@ static const struct ethtool_ops airoha_ethtool_ops = {
> > .get_drvinfo = airoha_ethtool_get_drvinfo,
> > .get_eth_mac_stats = airoha_ethtool_get_mac_stats,
> > .get_rmon_stats = airoha_ethtool_get_rmon_stats,
> > - .get_link_ksettings = phy_ethtool_get_link_ksettings,
> > + .get_link_ksettings = airoha_ethtool_get_link_ksettings,
> > + .set_link_ksettings = airoha_ethtool_set_link_ksettings,
> > .get_link = ethtool_op_get_link,
> > };
> >
> > @@ -3176,6 +3208,155 @@ bool airoha_is_valid_gdm_dev(struct airoha_eth *eth,
> > return false;
> > }
> >
> > +/* Nothing to do in MAC, everything is handled in PCS */
> > +static void airoha_mac_config(struct phylink_config *config, unsigned int mode,
> > + const struct phylink_link_state *state)
> > +{
> > +}
> > +
> > +static void airoha_mac_link_up(struct phylink_config *config, struct phy_device *phy,
> > + unsigned int mode, phy_interface_t interface,
> > + int speed, int duplex, bool tx_pause, bool rx_pause)
> > +{
> > + struct airoha_gdm_dev *dev = container_of(config, struct airoha_gdm_dev,
> > + phylink_config);
> > + struct airoha_gdm_port *port = dev->port;
> > + struct airoha_eth *eth = dev->eth;
> > + u32 frag_size_tx, frag_size_rx;
> > + u32 mask, val;
> > +
> > + /* TX/RX frag is configured only for GDM4 */
> > + if (port->id != AIROHA_GDM4_IDX)
> > + return;
> > +
> > + switch (speed) {
> > + case SPEED_10000:
> > + case SPEED_5000:
> > + frag_size_tx = 8;
> > + frag_size_rx = 8;
> > + break;
> > + case SPEED_2500:
> > + frag_size_tx = 2;
> > + frag_size_rx = 1;
> > + break;
> > + default:
> > + frag_size_tx = 1;
> > + frag_size_rx = 0;
> > + }
> > +
> > + spin_lock(&port->lock);
> > +
> > + /* Configure TX/RX frag based on speed */
> > + if (dev->nbq == 1) {
> > + mask = GDM4_SGMII1_TX_FRAG_SIZE_MASK;
> > + val = FIELD_PREP(GDM4_SGMII1_TX_FRAG_SIZE_MASK,
> > + frag_size_tx);
> > + } else {
> > + mask = GDM4_SGMII0_TX_FRAG_SIZE_MASK;
> > + val = FIELD_PREP(GDM4_SGMII0_TX_FRAG_SIZE_MASK,
> > + frag_size_tx);
> > + }
> > + airoha_fe_rmw(eth, REG_FE_GDM4_TMBI_FRAG, mask, val);
> > +
> > + if (dev->nbq == 1) {
> > + mask = GDM4_SGMII1_RX_FRAG_SIZE_MASK;
> > + val = FIELD_PREP(GDM4_SGMII1_RX_FRAG_SIZE_MASK,
> > + frag_size_rx);
> > + } else {
> > + mask = GDM4_SGMII0_RX_FRAG_SIZE_MASK;
> > + val = FIELD_PREP(GDM4_SGMII0_RX_FRAG_SIZE_MASK,
> > + frag_size_rx);
> > + }
> > + airoha_fe_rmw(eth, REG_FE_GDM4_RMBI_FRAG, mask, val);
> > +
> > + spin_unlock(&port->lock);
> > +}
> > +
> > +/* Nothing to do in MAC, everything is handled in PCS */
> > +static void airoha_mac_link_down(struct phylink_config *config, unsigned int mode,
> > + phy_interface_t interface)
> > +{
> > +}
> > +
> > +static const struct phylink_mac_ops airoha_phylink_ops = {
> > + .mac_config = airoha_mac_config,
> > + .mac_link_up = airoha_mac_link_up,
> > + .mac_link_down = airoha_mac_link_down,
> > +};
> > +
> > +static int airoha_fill_available_pcs(struct phylink_config *config,
> > + struct phylink_pcs **available_pcs,
> > + unsigned int num_possible_pcs)
> > +{
> > + struct device *dev = config->dev;
> > +
> > + return fwnode_phylink_pcs_parse(dev_fwnode(dev), available_pcs,
> > + num_possible_pcs);
> > +}
> > +
> > +static int airoha_setup_phylink(struct net_device *netdev)
> > +{
> > + struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > + struct device_node *np = netdev->dev.of_node;
> > + struct airoha_gdm_port *port = dev->port;
> > + struct phylink_config *config;
> > + phy_interface_t phy_mode;
> > + struct phylink *phylink;
> > + int err;
> > +
> > + err = of_get_phy_mode(np, &phy_mode);
> > + if (err) {
> > + dev_err(&netdev->dev, "incorrect phy-mode\n");
> > + return err;
> > + }
> > +
> > + config = &dev->phylink_config;
> > + config->dev = &netdev->dev;
> > + config->type = PHYLINK_NETDEV;
> > +
> > + /*
> > + * GDM1 only supports internal for Embedded Switch
> > + * and doesn't require a PCS.
> > + */
> > + if (port->id == AIROHA_GDM1_IDX) {
> > + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> > + MAC_10000FD;
> > +
> > + __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > + config->supported_interfaces);
> > + } else {
> > + config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> > + MAC_10 | MAC_100 | MAC_1000 |
> > + MAC_2500FD | MAC_5000FD | MAC_10000FD;
> > +
> > + config->num_possible_pcs = fwnode_phylink_pcs_count(dev_fwnode(config->dev));
> > + config->fill_available_pcs = airoha_fill_available_pcs;
> > +
> > + __set_bit(PHY_INTERFACE_MODE_SGMII,
> > + config->supported_interfaces);
> > + __set_bit(PHY_INTERFACE_MODE_1000BASEX,
> > + config->supported_interfaces);
> > + __set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > + config->supported_interfaces);
> > + __set_bit(PHY_INTERFACE_MODE_10GBASER,
> > + config->supported_interfaces);
> > + __set_bit(PHY_INTERFACE_MODE_USXGMII,
> > + config->supported_interfaces);
> > +
> > + phy_interface_copy(config->pcs_interfaces,
> > + config->supported_interfaces);
> > + }
> > +
> > + phylink = phylink_create(config, of_fwnode_handle(np),
> > + phy_mode, &airoha_phylink_ops);
> > + if (IS_ERR(phylink))
> > + return PTR_ERR(phylink);
> > +
> > + dev->phylink = phylink;
> > +
> > + return 0;
> > +}
> > +
> > static int airoha_alloc_gdm_device(struct airoha_eth *eth,
> > struct airoha_gdm_port *port,
> > int nbq, struct device_node *np)
> > @@ -3239,7 +3420,7 @@ static int airoha_alloc_gdm_device(struct airoha_eth *eth,
> > dev->nbq = nbq;
> > port->devs[index] = dev;
> >
> > - return 0;
> > + return airoha_setup_phylink(netdev);
> > }
> >
> > static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> > @@ -3274,7 +3455,7 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> > return -ENOMEM;
> >
> > port->id = id;
> > - spin_lock_init(&port->stats_lock);
> > + spin_lock_init(&port->lock);
> > eth->ports[p] = port;
> >
> > err = airoha_metadata_dst_alloc(port);
> > @@ -3471,6 +3652,8 @@ static int airoha_probe(struct platform_device *pdev)
> > netdev = netdev_from_priv(dev);
> > if (netdev->reg_state == NETREG_REGISTERED)
> > unregister_netdev(netdev);
> > + if (dev->phylink)
> > + phylink_destroy(dev->phylink);
> > of_node_put(netdev->dev.of_node);
> > }
> > airoha_metadata_dst_free(port);
> > @@ -3509,6 +3692,7 @@ static void airoha_remove(struct platform_device *pdev)
> >
> > netdev = netdev_from_priv(dev);
> > unregister_netdev(netdev);
> > + phylink_destroy(dev->phylink);
> > of_node_put(netdev->dev.of_node);
> > }
> > airoha_metadata_dst_free(port);
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> > index f6d01a8e8da1..b49fc5304b3a 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.h
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> > @@ -561,6 +561,9 @@ struct airoha_gdm_dev {
> > int nbq;
> >
> > struct airoha_hw_stats stats;
> > +
> > + struct phylink *phylink;
> > + struct phylink_config phylink_config;
> > };
> >
> > struct airoha_gdm_port {
> > @@ -568,8 +571,8 @@ struct airoha_gdm_port {
> > int id;
> > int users;
> >
> > - /* protect concurrent hw_stats accesses */
> > - spinlock_t stats_lock;
> > + /* protect concurrent hw_stats and frag register accesses */
> > + spinlock_t lock;
> >
> > struct metadata_dst *dsa_meta[AIROHA_MAX_DSA_PORTS];
> > };
> > diff --git a/drivers/net/ethernet/airoha/airoha_regs.h b/drivers/net/ethernet/airoha/airoha_regs.h
> > index 6fed63d013b4..8df02f51211c 100644
> > --- a/drivers/net/ethernet/airoha/airoha_regs.h
> > +++ b/drivers/net/ethernet/airoha/airoha_regs.h
> > @@ -357,6 +357,18 @@
> > #define IP_FRAGMENT_PORT_MASK GENMASK(8, 5)
> > #define IP_FRAGMENT_NBQ_MASK GENMASK(4, 0)
> >
> > +#define REG_FE_GDM4_TMBI_FRAG 0x2028
> > +#define GDM4_SGMII1_TX_WEIGHT_MASK GENMASK(31, 26)
> > +#define GDM4_SGMII1_TX_FRAG_SIZE_MASK GENMASK(25, 16)
> > +#define GDM4_SGMII0_TX_WEIGHT_MASK GENMASK(15, 10)
> > +#define GDM4_SGMII0_TX_FRAG_SIZE_MASK GENMASK(9, 0)
> > +
> > +#define REG_FE_GDM4_RMBI_FRAG 0x202c
> > +#define GDM4_SGMII1_RX_WEIGHT_MASK GENMASK(31, 26)
> > +#define GDM4_SGMII1_RX_FRAG_SIZE_MASK GENMASK(25, 16)
> > +#define GDM4_SGMII0_RX_WEIGHT_MASK GENMASK(15, 10)
> > +#define GDM4_SGMII0_RX_FRAG_SIZE_MASK GENMASK(9, 0)
> > +
> > #define REG_MC_VLAN_EN 0x2100
> > #define MC_VLAN_EN_MASK BIT(0)
> >
> > --
> > 2.53.0
> >
--
Ansuel
^ permalink raw reply
* RE: [PATCH net v8 2/3] tipc: fix NULL deref in deferred bulk distribution on publish failure
From: Tung Quang Nguyen @ 2026-07-20 15:44 UTC (permalink / raw)
To: Weiming Shi
Cc: Xiang Mei, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, kernel test robot, Jon Maloy,
netdev@vger.kernel.org, tipc-discussion@lists.sourceforge.net,
linux-kernel@vger.kernel.org
In-Reply-To: <20260718092544.785289-3-bestswngs@gmail.com>
>Subject: [PATCH net v8 2/3] tipc: fix NULL deref in deferred bulk distribution on
>publish failure
>
>tipc_net_finalize() does not check the return value of tipc_nametbl_publish().
>If the publish fails, for example on a GFP_ATOMIC allocation failure, the node
>state name never lands in cluster_scope, but tn->finalized is still set. A worker
>deferred by
>tipc_named_node_up() then wakes and calls named_distribute() with an
>empty list. That replays the same unguarded buf_msg(skb_peek_tail(list)) tail
>stamp, this time on the tipc_node_dist_bulk workqueue:
>
> KASAN: null-ptr-deref in range [0x00000000000000c8-0x00000000000000cf]
> RIP: 0010:named_distribute (net/tipc/name_distr.c:200)
> Workqueue: events tipc_node_dist_bulk
> Call Trace:
> tipc_named_dist_cluster_scope (net/tipc/name_distr.c:267)
> tipc_node_dist_bulk (net/tipc/node.c:403)
> process_one_work
> worker_thread
> Kernel panic - not syncing: Fatal exception in interrupt
>
>Check the publish result and warn on failure, but still set finalized, otherwise
>deferred workers would sleep forever. In
>tipc_named_dist_cluster_scope() re-check cluster_scope after the wait and
>skip the distribution when it is empty. This is a permanent condition, so return
>0 instead of an error, otherwise the link would be bounced forever. Also guard
>the tail stamp in named_distribute() itself, so a caller that misses the
>precondition gets a warning and a link reset through the existing -ENOBUFS
>path instead of a crash.
>
>Reproducing this needs an allocation failure during finalize, so I verified it by
>stubbing out the publish call: both nodes log the failure, the workers skip the
>distribution, no crash, no link flap.
>The normal path is unchanged with the same two-node test.
>
>Fixes: cad2929dc432 ("tipc: update a binding service via broadcast")
>Reported-by: Xiang Mei <xmei5@asu.edu>
>Assisted-by: Claude:claude-opus-4-8
>Signed-off-by: Weiming Shi <bestswngs@gmail.com>
Please drop this patch because sashiko reports the same issue for patch 1/3: https://sashiko.dev/#/patchset/20260718092544.785289-1-bestswngs%40gmail.com
^ permalink raw reply
* RE: [PATCH net v8 3/3] tipc: fix node reference leak when defer work is already pending
From: Tung Quang Nguyen @ 2026-07-20 15:45 UTC (permalink / raw)
To: Weiming Shi
Cc: Xiang Mei, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, linux-kernel@vger.kernel.org,
Jon Maloy, netdev@vger.kernel.org,
tipc-discussion@lists.sourceforge.net
In-Reply-To: <20260718092544.785289-4-bestswngs@gmail.com>
>Subject: [PATCH net v8 3/3] tipc: fix node reference leak when defer work is
>already pending
>
>In tipc_node_write_unlock(), TIPC_NOTIFY_NODE_UP with an empty
>cluster_scope takes a node reference and schedules n->work. If the link flaps
>down and up while that work is still pending, the next NODE_UP takes another
>reference, but schedule_work() returns false and the extra reference is never
>dropped. The tipc_node structure leaks.
>
>Verified by flapping the bearer while the work is pending: one reference is
>leaked per repeated NODE_UP, while the work is put only once when it finally
>runs.
>
>Drop the reference when the work was already queued.
>
>Reported-by: Xiang Mei <xmei5@asu.edu>
>Assisted-by: Claude:claude-opus-4-8
>Signed-off-by: Weiming Shi <bestswngs@gmail.com>
Please drop this patch because sashiko reports the same issue for patch 1/3: https://sashiko.dev/#/patchset/20260718092544.785289-1-bestswngs%40gmail.com
^ permalink raw reply
* Re: [PATCH v4 1/1] macvlan: allow source mode devices along with passthru
From: Simon Horman @ 2026-07-20 15:49 UTC (permalink / raw)
To: t.martitz
Cc: Simon Horman, andrew+netdev, davem, edumazet, kuba, pabeni,
netdev, linux-kernel
In-Reply-To: <20260713112414.2842803-2-t.martitz@fritz.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.
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260713112414.2842803-2-t.martitz@fritz.com?part=1
---
macvlan: allow source mode devices along with passthru
This change permits combining one passthru macvlan with one or more
source-mode macvlans on the same lower device, so that an rx_handler can
be attached to the passthru device while source-mode devices continue to
represent known senders.
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index c40fa331836b..635ed01abeb6 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
[ ... ]
> @@ -964,6 +969,21 @@ static int macvlan_init(struct net_device *dev)
> return 0;
> }
>
> +static void macvlan_restore_mac(struct macvlan_port *port)
> +{
> + /* If the lower device address has been changed by passthru
> + * macvlan, put it back.
> + */
> + if (macvlan_passthru(port) &&
> + !ether_addr_equal(port->dev->dev_addr, port->perm_addr)) {
> + struct sockaddr_storage ss;
> +
> + ss.ss_family = port->dev->type;
> + memcpy(&ss.__data, port->perm_addr, port->dev->addr_len);
> + dev_set_mac_address(port->dev, &ss, NULL);
> + }
> +}
> +
> static void macvlan_uninit(struct net_device *dev)
> {
> struct macvlan_dev *vlan = netdev_priv(dev);
> @@ -973,8 +993,19 @@ static void macvlan_uninit(struct net_device *dev)
>
> macvlan_flush_sources(port, vlan);
> port->count -= 1;
> - if (!port->count)
> - macvlan_port_destroy(port->dev);
> + if (port->count) {
> + /* In case of remaining source interfaces undo
> + * passthru-specific properties.
> + */
> + if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
> + /* Order is important, do not trigger macvlan_device_event(). */
> + macvlan_clear_passthru(port);
> + macvlan_restore_mac(port);
[High]
When the last passthru macvlan is removed while source-mode siblings
remain, does macvlan_restore_mac() actually restore the lower device's
MAC in this ordering?
macvlan_clear_passthru() is called first, which drops MACVLAN_F_PASSTHRU:
static inline void macvlan_clear_passthru(struct macvlan_port *port)
{
port->flags &= ~MACVLAN_F_PASSTHRU;
}
Then macvlan_restore_mac() runs, but its body is gated on
macvlan_passthru(port):
static void macvlan_restore_mac(struct macvlan_port *port)
{
if (macvlan_passthru(port) &&
!ether_addr_equal(port->dev->dev_addr, port->perm_addr)) {
...
dev_set_mac_address(port->dev, &ss, NULL);
}
}
Since the flag has just been cleared, does the guard evaluate to false
here, causing dev_set_mac_address() to be skipped and the lower device
to retain the passthru MAC instead of port->perm_addr?
The pre-patch macvlan_port_destroy() path did not clear the flag first,
so the restore actually took effect there.
> }
> + return;
> + }
> +
> + macvlan_port_destroy(port->dev);
> }
>
> static void macvlan_dev_get_stats64(struct net_device *dev,
[ ... ]
> @@ -1623,19 +1659,23 @@ static int macvlan_changelink(struct net_device *dev,
> if (data && data[IFLA_MACVLAN_MODE]) {
> set_mode = true;
> mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> - /* Passthrough mode can't be set or cleared dynamically */
> - if ((mode == MACVLAN_MODE_PASSTHRU) !=
> - (vlan->mode == MACVLAN_MODE_PASSTHRU))
> - return -EINVAL;
> - if (vlan->mode == MACVLAN_MODE_SOURCE &&
> - vlan->mode != mode)
> - macvlan_flush_sources(vlan->port, vlan);
> + if (mode != vlan->mode) {
> + /* Passthrough mode can't be set or cleared dynamically,
> + * regardless of existing source interfaces. Furthermore, source
> + * interfaces can't switch modes within a passhtrough port.
> + */
> + if (vlan->mode == MACVLAN_MODE_PASSTHRU ||
> + macvlan_passthru(vlan->port))
> + return -EINVAL;
> + if (vlan->mode == MACVLAN_MODE_SOURCE)
> + macvlan_flush_sources(vlan->port, vlan);
> + }
> }
[High]
Does this new check still reject setting an existing non-passthru vlan
to MACVLAN_MODE_PASSTHRU when the port has no passthru vlan yet?
The comment still says "Passthrough mode can't be set or cleared
dynamically", but the condition:
if (vlan->mode == MACVLAN_MODE_PASSTHRU ||
macvlan_passthru(vlan->port))
return -EINVAL;
only catches clearing passthru on a passthru vlan, and any mode change
on a vlan whose port already has passthru.
If vlan->mode is one of VEPA/BRIDGE/PRIVATE/SOURCE and the request is
mode == MACVLAN_MODE_PASSTHRU on a port that has no passthru vlan yet,
does execution fall through? Later in the same function:
if (set_mode)
vlan->mode = mode;
promotes vlan->mode to PASSTHRU, but nothing calls
macvlan_set_passthru(port), nothing performs eth_hw_addr_inherit(dev,
lowerdev), and no passthru promiscuity accounting is done.
After this, does the vlan report mode == PASSTHRU while the port flag
MACVLAN_F_PASSTHRU stays clear? Downstream code splits between the two
predicates:
- macvlan_open, macvlan_stop, macvlan_fdb_add, macvlan_fdb_del, and
the IFLA_MACVLAN_FLAGS branch here read vlan->mode ==
MACVLAN_MODE_PASSTHRU.
- macvlan_device_event NETDEV_CHANGEADDR, macvlan_addr_busy,
macvlan_change_rx_flags, macvlan_sync_address and
macvlan_handle_frame read macvlan_passthru(port).
Does this then produce visible symptoms such as:
- macvlan_open calling dev_set_promiscuity(lowerdev, 1) while the
lower device's MAC was never inherited to the vlan;
- macvlan_addr_busy no longer treating the port as passthru, allowing
a sibling with the lowerdev's address;
- macvlan_handle_frame not delivering to this vlan via
list_first_or_null_rcu(&port->vlans) because the port flag is
clear?
Would a condition that also rejects the direction into passthru be more
consistent with the comment, e.g.:
if (vlan->mode == MACVLAN_MODE_PASSTHRU ||
mode == MACVLAN_MODE_PASSTHRU ||
macvlan_passthru(vlan->port))
return -EINVAL;
>
> if (data && data[IFLA_MACVLAN_FLAGS]) {
> __u16 flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
> bool promisc = (flags ^ vlan->flags) & MACVLAN_FLAG_NOPROMISC;
> - if (macvlan_passthru(vlan->port) && promisc) {
> + if (vlan->mode == MACVLAN_MODE_PASSTHRU && promisc) {
> int err;
>
> if (flags & MACVLAN_FLAG_NOPROMISC)
^ permalink raw reply
* [PATCH net] xsk: reject tx_metadata_len smaller than struct xsk_tx_metadata
From: Cen Zhang (Microsoft) @ 2026-07-20 15:52 UTC (permalink / raw)
To: magnus.karlsson, maciej.fijalkowski, davem, edumazet, kuba,
pabeni
Cc: sdf, horms, netdev, bpf, linux-kernel, AutonomousCodeSecurity,
tgopinath, kys, blbllhy
xdp_umem_reg() validates tx_metadata_len for upper bound (<256) and
alignment (%8) but not a lower bound. xsk_skb_metadata() computes
meta = buffer - pool->tx_metadata_len then unconditionally accesses
the full 24-byte struct xsk_tx_metadata, so any value less than
sizeof(struct xsk_tx_metadata) allows an out-of-bounds read.
KASAN reports this as:
BUG: KASAN: vmalloc-out-of-bounds in xsk_skb_metadata+0x4b2/0x500
Read of size 8 at addr ffffc90000f11000 by task exploit/148
xsk_skb_metadata (net/xdp/xsk.c:837)
xsk_build_skb (net/xdp/xsk.c)
__xsk_generic_xmit (net/xdp/xsk.c)
xsk_sendmsg (net/xdp/xsk.c)
Add a lower-bound check in xdp_umem_reg() to reject tx_metadata_len
values that cannot cover the full metadata struct.
Fixes: 341ac980eab9 ("xsk: Support tx_metadata_len")
Reported-by: AutonomousCodeSecurity@microsoft.com
Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
---
net/xdp/xdp_umem.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
index 58da2f4f4397..d16ad9d8f919 100644
--- a/net/xdp/xdp_umem.c
+++ b/net/xdp/xdp_umem.c
@@ -208,7 +208,8 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
return -EINVAL;
if (mr->flags & XDP_UMEM_TX_METADATA_LEN) {
- if (mr->tx_metadata_len >= 256 || mr->tx_metadata_len % 8)
+ if (mr->tx_metadata_len < sizeof(struct xsk_tx_metadata) ||
+ mr->tx_metadata_len >= 256 || mr->tx_metadata_len % 8)
return -EINVAL;
umem->tx_metadata_len = mr->tx_metadata_len;
}
--
2.53.0
^ permalink raw reply related
* RE: [Intel-wired-lan] [PATCH iwl-next v3 1/2] idpf: remove conditional MBX deinit from idpf_vc_core_deinit()
From: Salin, Samuel @ 2026-07-20 15:59 UTC (permalink / raw)
To: Tantilov, Emil S, intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org, Kitszel, Przemyslaw, Bhat, Jay,
Barrera, Ivan D, Loktionov, Aleksandr, Zaremba, Larysa,
Nguyen, Anthony L, andrew+netdev@lunn.ch, davem@davemloft.net,
edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
Lobakin, Aleksander, linux-pci@vger.kernel.org, Chittim, Madhu,
decot@google.com, willemb@google.com, sheenamo@google.com,
lukas@wunner.de
In-Reply-To: <20260630231854.11536-2-emil.s.tantilov@intel.com>
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Emil Tantilov
> Sent: Tuesday, June 30, 2026 4:19 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Bhat, Jay <jay.bhat@intel.com>; Barrera,
> Ivan D <ivan.d.barrera@intel.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Zaremba, Larysa
> <larysa.zaremba@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; Lobakin, Aleksander <aleksander.lobakin@intel.com>;
> linux-pci@vger.kernel.org; Chittim, Madhu <madhu.chittim@intel.com>;
> decot@google.com; willemb@google.com; sheenamo@google.com;
> lukas@wunner.de
> Subject: [Intel-wired-lan] [PATCH iwl-next v3 1/2] idpf: remove conditional
> MBX deinit from idpf_vc_core_deinit()
>
> Previously it was assumed that idpf_vc_core_deinit() is always being called
> during reset handling, where the MBX is disabled by the reset, with remove
> being the exception. Ideally the driver needs to communicate the changes to
> FW in all instances where the MBX is not already disabled.
> Remove the remove_in_prog check from idpf_vc_core_deinit() as the MBX
> was already disabled while handling the reset via libie_ctlq_xn_shutdown() in
> the service task. This is also needed by the following patch, introducing PCI
> callbacks support, specifically in the case where FLR is being triggered by a
> user, in which case, the driver still has the ability to notify FW before the reset
> happens.
>
> Add call to libie_ctlq_xn_shutdown() in idpf_shutdown() to avoid a possible
> regression where long timeouts can happen on shutdown when FW is down.
>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Reviewed-by: Jay Bhat <jay.bhat@intel.com>
> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
> 2.37.3
Tested-by: Samuel Salin <Samuel.salin@intel.com>
^ permalink raw reply
* Re: [PATCH 7/8] net: mv643xx: use platform_device_set_fwnode()
From: Bartosz Golaszewski @ 2026-07-20 16:01 UTC (permalink / raw)
To: Andrew Lunn
Cc: Bartosz Golaszewski, Greg Kroah-Hartman, Rafael J. Wysocki,
Danilo Krummrich, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy (CS GROUP), Andi Shyti,
Joerg Roedel (AMD), Will Deacon, Robin Murphy, Andy Shevchenko,
Doug Berger, Florian Fainelli,
Broadcom internal kernel review list, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Ulf Hansson, Frank Li, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, Lee Jones, Sebastian Hesselbarth,
Srinivas Kandagatla, brgl, driver-core, linuxppc-dev,
linux-kernel, linux-i2c, iommu, netdev, linux-pm, imx,
linux-arm-kernel, mfd, linux-arm-msm, linux-sound
In-Reply-To: <86b2aba7-b049-47e6-bc94-6cb499b30ce4@lunn.ch>
On Mon, 20 Jul 2026 16:43:40 +0200, Andrew Lunn <andrew@lunn.ch> said:
> On Mon, Jul 20, 2026 at 11:24:54AM +0200, Bartosz Golaszewski wrote:
>> Prefer the higher-level platform_device_set_fwnode() over the
>> OF-specific platform_device_set_of_node() for dynamically allocated
>> platform devices.
>>
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
>> ---
>> drivers/net/ethernet/marvell/mv643xx_eth.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
>> index 9caa1e47c174c9d7a161b7f2e2ee12a829b813d4..2f2d6cce8d852b9ec3ab42678a04a7915d1f00cc 100644
>> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
>> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
>> @@ -2780,7 +2780,7 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev,
>> goto put_err;
>> }
>> ppdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>> - platform_device_set_of_node(ppdev, pnp);
>> + platform_device_set_fwnode(ppdev, of_fwnode_handle(pnp));
>
> This is definitely an OF only driver. There are no other calls to
> fwnode functions in this driver, so this is the wrong thing to do.
>
> Sorry, NACK.
>
I'm not going to die on this hill but drivers are OF-only until they're not.
For example, Qualcomm is now working on a hybrid ACPI-OF approach for
laptops[1] and we may end up needing to start converting drivers to fwnode
after all.
There's no real benefit to sticking to OF-specific APIs unless you need to
iterate over all properties of a node or use some other functionality not
available in fwnode. The overhead is minimal and it's never a hot path.
Thanks,
Bartosz
[1] https://lore.kernel.org/all/20260623145225.143218-1-johannes.goede@oss.qualcomm.com/
^ permalink raw reply
* [PATCH net] vxlan: mdb: Fix source list corruption on a failed replace
From: James Raphael Tiovalen @ 2026-07-20 16:04 UTC (permalink / raw)
To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev
Cc: James Raphael Tiovalen, stable, Kees Cook, Nikolay Aleksandrov,
Ido Schimmel, linux-kernel
When replacing the source list of an MDB remote entry, all existing
sources are first marked for deletion and vxlan_mdb_remote_srcs_add()
is then called to add the new source list. Sources present in the new
list have their deletion mark cleared, and any sources left marked
afterwards are removed.
If vxlan_mdb_remote_srcs_add() fails partway through, its error path
deletes all entries on the remote's source list. That rollback is only
correct for its other caller, vxlan_mdb_remote_add(), where the remote
was just allocated and the list contains solely entries added during
the call. On the replace path the list also holds pre-existing sources,
so a failed replace tears them down together with their (S, G)
forwarding entries instead of leaving the entry unchanged.
This is reachable from an existing (*, G) remote. An EXCLUDE filter
that loses sources starts forwarding traffic that should be blocked,
while an INCLUDE filter that loses sources drops traffic that should be
forwarded.
Mark entries created during the current pass with a new
VXLAN_SGRP_F_NEW flag. On failure, delete only those entries and clear
the deletion mark on the pre-existing ones, so a failed replace leaves
the source list untouched. Retain the flag until the whole operation
succeeds and then clear it. Also stop vxlan_mdb_remote_src_add() from
deleting a pre-existing entry it only looked up when adding that
entry's forwarding entry fails.
Fixes: a3a48de5eade ("vxlan: mdb: Add MDB control path support")
Cc: stable@vger.kernel.org
Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
drivers/net/vxlan/vxlan_mdb.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/net/vxlan/vxlan_mdb.c b/drivers/net/vxlan/vxlan_mdb.c
index 055a4969f593..af7a0d7f95a5 100644
--- a/drivers/net/vxlan/vxlan_mdb.c
+++ b/drivers/net/vxlan/vxlan_mdb.c
@@ -42,6 +42,7 @@ struct vxlan_mdb_remote {
};
#define VXLAN_SGRP_F_DELETE BIT(0)
+#define VXLAN_SGRP_F_NEW BIT(1)
struct vxlan_mdb_src_entry {
struct hlist_node node;
@@ -844,6 +845,7 @@ vxlan_mdb_remote_src_add(const struct vxlan_mdb_config *cfg,
ent = vxlan_mdb_remote_src_entry_add(remote, &src->addr);
if (!ent)
return -ENOMEM;
+ ent->flags |= VXLAN_SGRP_F_NEW;
} else if (!(cfg->nlflags & NLM_F_REPLACE)) {
NL_SET_ERR_MSG_MOD(extack, "Source entry already exists");
return -EEXIST;
@@ -853,15 +855,16 @@ vxlan_mdb_remote_src_add(const struct vxlan_mdb_config *cfg,
if (err)
goto err_src_del;
- /* Clear flags in case source entry was marked for deletion as part of
- * replace flow.
+ /* Clear the deletion mark so the entry survives the replace sweep.
+ * The new mark is retained until the whole operation succeeds.
*/
- ent->flags = 0;
+ ent->flags &= ~VXLAN_SGRP_F_DELETE;
return 0;
err_src_del:
- vxlan_mdb_remote_src_entry_del(ent);
+ if (ent->flags & VXLAN_SGRP_F_NEW)
+ vxlan_mdb_remote_src_entry_del(ent);
return err;
}
@@ -889,11 +892,19 @@ static int vxlan_mdb_remote_srcs_add(const struct vxlan_mdb_config *cfg,
goto err_src_del;
}
+ hlist_for_each_entry(ent, &remote->src_list, node)
+ ent->flags &= ~VXLAN_SGRP_F_NEW;
+
return 0;
err_src_del:
- hlist_for_each_entry_safe(ent, tmp, &remote->src_list, node)
- vxlan_mdb_remote_src_del(cfg->vxlan, &cfg->group, remote, ent);
+ hlist_for_each_entry_safe(ent, tmp, &remote->src_list, node) {
+ if (ent->flags & VXLAN_SGRP_F_NEW)
+ vxlan_mdb_remote_src_del(cfg->vxlan, &cfg->group, remote,
+ ent);
+ else
+ ent->flags &= ~VXLAN_SGRP_F_DELETE;
+ }
return err;
}
@@ -1069,7 +1080,7 @@ vxlan_mdb_remote_srcs_replace(const struct vxlan_mdb_config *cfg,
err = vxlan_mdb_remote_srcs_add(cfg, remote, extack);
if (err)
- goto err_clear_delete;
+ return err;
hlist_for_each_entry_safe(ent, tmp, &remote->src_list, node) {
if (ent->flags & VXLAN_SGRP_F_DELETE)
@@ -1078,11 +1089,6 @@ vxlan_mdb_remote_srcs_replace(const struct vxlan_mdb_config *cfg,
}
return 0;
-
-err_clear_delete:
- hlist_for_each_entry(ent, &remote->src_list, node)
- ent->flags &= ~VXLAN_SGRP_F_DELETE;
- return err;
}
static int vxlan_mdb_remote_replace(const struct vxlan_mdb_config *cfg,
--
2.43.0
^ permalink raw reply related
* Re: [PATCH net-next v5 05/15] libie: add bookkeeping support for control queue messages
From: Larysa Zaremba @ 2026-07-20 16:07 UTC (permalink / raw)
To: Tony Nguyen
Cc: davem, kuba, pabeni, edumazet, andrew+netdev, netdev,
Phani R Burra, przemyslaw.kitszel, aleksander.lobakin,
sridhar.samudrala, michal.swiatkowski, maciej.fijalkowski,
emil.s.tantilov, madhu.chittim, joshua.a.hay, jacob.e.keller,
jayaprakash.shanmugam, jiri, horms, corbet, richardcochran, skhan,
linux-doc, Bharath R, Samuel Salin
In-Reply-To: <20260715180042.1972010-6-anthony.l.nguyen@intel.com>
Sashiko says:
> > + wait_for_completion_timeout(&xn->cmd_completion_event,
> > + msecs_to_jiffies(params->timeout_ms));
> > +
> > + spin_lock(&xn->xn_lock);
> > + switch (xn->state) {
> > + case LIBIE_CTLQ_XN_WAITING:
> > + ret = -ETIMEDOUT;
> > + break;
> > + case LIBIE_CTLQ_XN_COMPLETED_SUCCESS:
> > + params->recv_mem = xn->recv_mem;
> > + break;
> > + default:
> > + ret = -EBADMSG;
> > + break;
> > + }
> > +
> > + /* Free the receive buffer in case of failure. On timeout, receive
> > + * buffer is not allocated.
> > + */
> > + if (ret && ret != -ETIMEDOUT)
> > + libie_ctlq_release_rx_buf(&xn->recv_mem);
> Can this cause a double free on a stale receive buffer if the transaction was
> aborted? When a transaction is returned to the free pool, xn->recv_mem is not
> cleared. If it is reallocated and enters the WAITING state, it still contains
> the recv_mem pointer from its last successful use. If libie_ctlq_xn_shutdown()
> aborts this transaction, its state changes to SHUTDOWN. This switch falls through
> to the default case setting ret = -EBADMSG, and this cleanup logic will call
> libie_ctlq_release_rx_buf() on the stale recv_mem pointer.
LIBIE_CTLQ_XN_WAITING can only occur in the previous critical sections,
since it's only applicable to idle transactions.
[...]
>
> > + wait_for_completion_timeout(&xn->cmd_completion_event,
> > + msecs_to_jiffies(params->timeout_ms));
> params->timeout_ms is declared u64 in struct libie_ctlq_xn_send_params
> (and stored to xn->timeout_ms which is also u64), but msecs_to_jiffies()
> takes const unsigned int. On 64-bit builds, values above UINT_MAX get
> silently truncated to 32 bits before conversion, and the MAX_JIFFY_OFFSET
> clamp inside _msecs_to_jiffies() only fires against a value that already
> fits in int, so it can't rescue the truncation.
> The async path uses ktime_ms_delta against a u64 correctly, so the sync
> and async paths would disagree for the same input.
> Should timeout_ms be u32 in both structures (matching msecs_to_jiffies()),
> or should the value be clamped before conversion here?
>
> > + if (!libie_cp_can_send_onstack(msg->data_len)) {
> > + libie_cp_unmap_dma_mem(dev, &msg->send_mem);
> > + params->rel_tx_buf(msg->send_mem.va);
> [Medium, Medium]
> Two questions about locking context here.
> First, params->rel_tx_buf() is being called while ctlq->lock (a plain
> spin_lock) is held. In libie_ctlq_xn_send() the same callback is invoked
> at the free_buf label with no locks held, so the two libie call sites
> impose different atomic-context constraints on the same driver-supplied
> callback. Should rel_tx_buf either be moved outside the critical section
> or explicitly documented as required to be non-sleeping?
I see no reason to do this currently
> Second, xnm->free_xns_bm_lock, xn->xn_lock, and ctlq->lock are all
> acquired with plain spin_lock() throughout the new code. If a future
> consumer drives libie_ctlq_xn_recv() from softirq (NAPI-like, which the
> budget parameter and page_pool usage suggest) while libie_ctlq_xn_send()
> runs from process context on the same CPU, wouldn't the softirq deadlock
> against the process-context lock holder? Would spin_lock_bh() variants,
> or an explicit documented callable-context contract, be preferable?
No softirqs
[...]
>
> > +/**
> > + * libie_ctlq_xn_deinit - deallocate and free the transaction manager resources
> > + * @xnm: pointer to the transaction manager
> > + * @ctx: controlq context structure
> > + *
> > + * All Rx processing must be stopped beforehand.
> > + */
> > +void libie_ctlq_xn_deinit(struct libie_ctlq_xn_manager *xnm,
> > + struct libie_ctlq_ctx *ctx)
> > +{
> > + libie_ctlq_xn_shutdown(xnm);
> > + libie_ctlq_xn_deinit_dma(&ctx->mmio_info.pdev->dev, xnm,
> > + LIBIE_CTLQ_MAX_XN_ENTRIES);
> > + kfree(xnm);
> > + libie_ctlq_deinit(ctx);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(libie_ctlq_xn_deinit, "LIBIE_CP");
> [High, Medium]
> Two concerns about this deinit path.
> First, do large in-flight sends leak DMA mappings and TX buffers?
> libie_ctlq_xn_shutdown() only manages xn slot state and does not touch
> the TX descriptor ring. libie_ctlq_xn_deinit_dma() only walks
> xn->dma_mem (the copybreak buffers), not ctlq_msg->send_mem for larger
> sends. libie_ctlq_deinit() eventually reaches libie_ctlq_free_tx_msgs()
> which does only kfree(ctlq->tx_msg[i]).
> Nothing on this path calls libie_cp_unmap_dma_mem() on msg->send_mem or
> invokes rel_tx_buf() for a msg->send_mem.va that a caller supplied via
> kmalloc. Wouldn't every posted-but-not-yet-completion-cleaned large send
> leak its DMA mapping and buffer? The kerneldoc only requires "All Rx
> processing must be stopped beforehand" and doesn't warn that
> libie_ctlq_xn_send_clean(force=true) must be called first.
Could be phrased better, but idpf and ixd use the API correctly, so no
real consequences.
^ permalink raw reply
* Re: [PATCH net-next v5 08/15] idpf: refactor idpf to use libie_pci APIs
From: Larysa Zaremba @ 2026-07-20 16:09 UTC (permalink / raw)
To: Tony Nguyen
Cc: davem, kuba, pabeni, edumazet, andrew+netdev, netdev,
Pavan Kumar Linga, przemyslaw.kitszel, aleksander.lobakin,
sridhar.samudrala, michal.swiatkowski, maciej.fijalkowski,
emil.s.tantilov, joshua.a.hay, jacob.e.keller,
jayaprakash.shanmugam, jiri, horms, corbet, richardcochran, skhan,
linux-doc, Samuel Salin
In-Reply-To: <20260715180042.1972010-9-anthony.l.nguyen@intel.com>
Sashiko says:
> > + wait_for_completion_timeout(&xn->cmd_completion_event,
> > + msecs_to_jiffies(params->timeout_ms));
> > +
> > + spin_lock(&xn->xn_lock);
> > + switch (xn->state) {
> > + case LIBIE_CTLQ_XN_WAITING:
> > + ret = -ETIMEDOUT;
> > + break;
> > + case LIBIE_CTLQ_XN_COMPLETED_SUCCESS:
> > + params->recv_mem = xn->recv_mem;
> > + break;
> > + default:
> > + ret = -EBADMSG;
> > + break;
> > + }
> > +
> > + /* Free the receive buffer in case of failure. On timeout, receive
> > + * buffer is not allocated.
> > + */
> > + if (ret && ret != -ETIMEDOUT)
> > + libie_ctlq_release_rx_buf(&xn->recv_mem);
> Can this cause a double free on a stale receive buffer if the transaction was
> aborted? When a transaction is returned to the free pool, xn->recv_mem is not
> cleared. If it is reallocated and enters the WAITING state, it still contains
> the recv_mem pointer from its last successful use. If libie_ctlq_xn_shutdown()
> aborts this transaction, its state changes to SHUTDOWN. This switch falls through
> to the default case setting ret = -EBADMSG, and this cleanup logic will call
> libie_ctlq_release_rx_buf() on the stale recv_mem pointer.
LIBIE_CTLQ_XN_WAITING can only occur in the previous critical sections,
since it's only applicable to idle transactions.
>
> > + wait_for_completion_timeout(&xn->cmd_completion_event,
> > + msecs_to_jiffies(params->timeout_ms));
> params->timeout_ms is declared u64 in struct libie_ctlq_xn_send_params
> (and stored to xn->timeout_ms which is also u64), but msecs_to_jiffies()
> takes const unsigned int. On 64-bit builds, values above UINT_MAX get
> silently truncated to 32 bits before conversion, and the MAX_JIFFY_OFFSET
> clamp inside _msecs_to_jiffies() only fires against a value that already
> fits in int, so it can't rescue the truncation.
> The async path uses ktime_ms_delta against a u64 correctly, so the sync
> and async paths would disagree for the same input.
> Should timeout_ms be u32 in both structures (matching msecs_to_jiffies()),
> or should the value be clamped before conversion here?
>
> > + if (!libie_cp_can_send_onstack(msg->data_len)) {
> > + libie_cp_unmap_dma_mem(dev, &msg->send_mem);
> > + params->rel_tx_buf(msg->send_mem.va);
> [Medium, Medium]
> Two questions about locking context here.
> First, params->rel_tx_buf() is being called while ctlq->lock (a plain
> spin_lock) is held. In libie_ctlq_xn_send() the same callback is invoked
> at the free_buf label with no locks held, so the two libie call sites
> impose different atomic-context constraints on the same driver-supplied
> callback. Should rel_tx_buf either be moved outside the critical section
> or explicitly documented as required to be non-sleeping?
I see no reason to do this currently
> Second, xnm->free_xns_bm_lock, xn->xn_lock, and ctlq->lock are all
> acquired with plain spin_lock() throughout the new code. If a future
> consumer drives libie_ctlq_xn_recv() from softirq (NAPI-like, which the
> budget parameter and page_pool usage suggest) while libie_ctlq_xn_send()
> runs from process context on the same CPU, wouldn't the softirq deadlock
> against the process-context lock holder? Would spin_lock_bh() variants,
> or an explicit documented callable-context contract, be preferable?
No softirqs
>
> > +/**
> > + * libie_ctlq_xn_deinit - deallocate and free the transaction manager resources
> > + * @xnm: pointer to the transaction manager
> > + * @ctx: controlq context structure
> > + *
> > + * All Rx processing must be stopped beforehand.
> > + */
> > +void libie_ctlq_xn_deinit(struct libie_ctlq_xn_manager *xnm,
> > + struct libie_ctlq_ctx *ctx)
> > +{
> > + libie_ctlq_xn_shutdown(xnm);
> > + libie_ctlq_xn_deinit_dma(&ctx->mmio_info.pdev->dev, xnm,
> > + LIBIE_CTLQ_MAX_XN_ENTRIES);
> > + kfree(xnm);
> > + libie_ctlq_deinit(ctx);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(libie_ctlq_xn_deinit, "LIBIE_CP");
> [High, Medium]
> Two concerns about this deinit path.
> First, do large in-flight sends leak DMA mappings and TX buffers?
> libie_ctlq_xn_shutdown() only manages xn slot state and does not touch
> the TX descriptor ring. libie_ctlq_xn_deinit_dma() only walks
> xn->dma_mem (the copybreak buffers), not ctlq_msg->send_mem for larger
> sends. libie_ctlq_deinit() eventually reaches libie_ctlq_free_tx_msgs()
> which does only kfree(ctlq->tx_msg[i]).
> Nothing on this path calls libie_cp_unmap_dma_mem() on msg->send_mem or
> invokes rel_tx_buf() for a msg->send_mem.va that a caller supplied via
> kmalloc. Wouldn't every posted-but-not-yet-completion-cleaned large send
> leak its DMA mapping and buffer? The kerneldoc only requires "All Rx
> processing must be stopped beforehand" and doesn't warn that
> libie_ctlq_xn_send_clean(force=true) must be called first.
Could be phrased better, but idpf and ixd use the API correctly.
^ permalink raw reply
* Re: [PATCH v3 net] octeontx2-af: Block VFs from clobbering special CGX PKIND state
From: Simon Horman @ 2026-07-20 16:09 UTC (permalink / raw)
To: rkannoth
Cc: Simon Horman, davem, gakula, linux-kernel, netdev, sgoutham,
andrew+netdev, edumazet, kuba, pabeni, hkelam
In-Reply-To: <20260713121902.3938875-1-rkannoth@marvell.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.
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260713121902.3938875-1-rkannoth@marvell.com?part=1
---
octeontx2-af: Block VFs from clobbering special CGX PKIND state
This commit prevents CGX-mapped VFs from resetting the shared LMAC RX
PKIND and default TX parse configuration when the LMAC is running with
HiGig2 or EDSA parsing enabled. It adds cgx_get_pkind() and
rvu_cgx_is_pkind_config_permitted() to gate the affected register
writes.
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> index 4ff3935ed3fe..2be1da3476ac 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cgx.c
> @@ -1355,3 +1355,35 @@ void rvu_mac_reset(struct rvu *rvu, u16 pcifunc)
> if (mac_ops->mac_reset(cgxd, lmac, !is_vf(pcifunc)))
> dev_err(rvu->dev, "Failed to reset MAC\n");
> }
> +
> +/* Do not allow CGX-mapped VFs to overwrite PKIND when special parse kinds
> + * (HiGig, EDSA, etc.) are in use on the shared LMAC.
> + */
> +bool rvu_cgx_is_pkind_config_permitted(struct rvu *rvu, u16 pcifunc)
> +{
> + int pf, err, rxpkind;
> + u8 cgx_id, lmac_id;
> + void *cgxd;
> +
> + pf = rvu_get_pf(rvu->pdev, pcifunc);
> +
> + if (!(pcifunc & RVU_PFVF_FUNC_MASK))
> + return true;
> +
> + if (!is_pf_cgxmapped(rvu, pf))
> + return true;
> +
> + rvu_get_cgx_lmac_id(rvu->pf2cgxlmac_map[pf], &cgx_id, &lmac_id);
> + cgxd = rvu_cgx_pdata(cgx_id, rvu);
> + err = cgx_get_pkind(cgxd, lmac_id, &rxpkind);
> + if (err)
> + return false;
> +
> + switch (rxpkind) {
> + case NPC_RX_HIGIG_PKIND:
> + case NPC_RX_EDSA_PKIND:
> + return false;
> + default:
> + return true;
> + }
> +}
[Medium]
The three call sites of rvu_cgx_is_pkind_config_permitted() interpret a
false return in two different ways. In nix_interface_init() and
rvu_mbox_handler_nix_lf_alloc() a false result silently skips the write,
but the TX branch of rvu_npc_set_parse_mode() converts it into an
-EINVAL to the caller.
Pre-patch, rvu_mbox_handler_npc_set_pkind() (via rvu_npc_set_parse_mode)
always wrote NIX_AF_LFX_TX_PARSE_CFG. Post-patch, a VF whose parent
PF's LMAC is programmed with NPC_RX_HIGIG_PKIND or NPC_RX_EDSA_PKIND
now receives a hard error where the call previously succeeded.
The same path is also reached from rvu_nix_lf_teardown() ->
rvu_npc_set_parse_mode(), where the -EINVAL is ignored but the intended
reset write to NIX_AF_LFX_TX_PARSE_CFG is skipped.
Is this mbox-level behavior change intended? The commit message uses
the word "skip", which reads like a silent no-op, but this call site
aborts with -EINVAL rather than skipping.
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> index 6a0ce2665031..040eb4c9b8d1 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
[ ... ]
> @@ -1685,8 +1693,10 @@ int rvu_mbox_handler_nix_lf_alloc(struct rvu *rvu,
> rvu_write64(rvu, blkaddr, NIX_AF_LFX_RX_CFG(nixlf), req->rx_cfg);
>
> /* Configure pkind for TX parse config */
> - cfg = NPC_TX_DEF_PKIND;
> - rvu_write64(rvu, blkaddr, NIX_AF_LFX_TX_PARSE_CFG(nixlf), cfg);
> + if (rvu_cgx_is_pkind_config_permitted(rvu, pcifunc)) {
> + cfg = NPC_TX_DEF_PKIND;
> + rvu_write64(rvu, blkaddr, NIX_AF_LFX_TX_PARSE_CFG(nixlf), cfg);
> + }
[Medium]
This check-then-write pair reads the CGX RX pkind register (via
cgx_get_pkind() inside rvu_cgx_is_pkind_config_permitted()) without
holding cgxd->lock, but the corresponding cgx_set_pkind() writers were
just wrapped in cgxd->lock in this same patch.
Consider the following interleaving:
CPU0 (VF nix_lf_alloc) CPU1 (PF set_parse_mode)
----------------------------------- ---------------------------
rvu_cgx_is_pkind_config_permitted()
cgx_get_pkind() -> normal pkind
returns true
mutex_lock(&cgxd->lock);
cgx_set_pkind(..., HIGIG);
mutex_unlock(&cgxd->lock);
rvu_write64(NIX_AF_LFX_TX_PARSE_CFG,
NPC_TX_DEF_PKIND);
Can a concurrent PF that transitions the LMAC to NPC_RX_HIGIG_PKIND or
NPC_RX_EDSA_PKIND between the check and the write flip the state the
check depends on?
The nix_interface_init() call site holds cgxd->lock across both the
check and the writes; should the same pattern be applied here (and in
the TX branch of rvu_npc_set_parse_mode()) for consistency?
[Medium]
NIX_AF_LFX_TX_PARSE_CFG(nixlf) is indexed by NIX LF, not by LMAC.
Skipping the write when the LMAC is in HiGig/EDSA mode means the
register is left at whatever value it currently holds — either the
hardware reset value on a fresh LF, or stale contents from a previous
owner when an LF is reused across configurations.
If a HiGig VF needs NIX_AF_LFX_TX_PARSE_CFG programmed (for example to
NPC_TX_HIGIG_PKIND) to emit correctly formatted frames, where is that
value installed? This hunk unconditionally elides the write when the
LMAC is HiGig/EDSA, and the teardown path via rvu_nix_lf_teardown() ->
rvu_npc_set_parse_mode() is likewise blocked from restoring the
default, so a subsequent non-HiGig owner reusing the same LF would
observe the previous owner's TX parse configuration.
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> index c7bc0b3a29b9..1297c6681a01 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> @@ -4241,6 +4245,9 @@ int rvu_npc_set_parse_mode(struct rvu *rvu, u16 pcifunc, u64 mode, u8 dir,
> if (rc)
> return rc;
>
> + if (!rvu_cgx_is_pkind_config_permitted(rvu, pcifunc))
> + return -EINVAL;
> +
> rvu_write64(rvu, blkaddr, NIX_AF_LFX_TX_PARSE_CFG(nixlf),
> txpkind);
> }
^ permalink raw reply
* Re: [PATCH net-next v5 09/15] idpf: refactor idpf to use libie control queues
From: Larysa Zaremba @ 2026-07-20 16:11 UTC (permalink / raw)
To: Tony Nguyen
Cc: davem, kuba, pabeni, edumazet, andrew+netdev, netdev,
Pavan Kumar Linga, przemyslaw.kitszel, aleksander.lobakin,
sridhar.samudrala, michal.swiatkowski, maciej.fijalkowski,
emil.s.tantilov, joshua.a.hay, jacob.e.keller,
jayaprakash.shanmugam, jiri, horms, corbet, richardcochran, skhan,
linux-doc, Aleksandr Loktionov, Samuel Salin
In-Reply-To: <20260715180042.1972010-10-anthony.l.nguyen@intel.com>
Sashiko says:
> > +void idpf_send_vf_reset_msg(struct idpf_adapter *adapter)
> > +{
> > + struct libie_ctlq_info *ctlq = adapter->asq;
> > +
> > + /* Forcefully claim send queue slot */
> > + idpf_mb_clean(adapter, ctlq, true);
> > +
> > + scoped_guard(spinlock, &ctlq->lock) {
> > + *ctlq->tx_msg[ctlq->next_to_use] = (struct libie_ctlq_msg) {
> > + .opcode = LIBIE_CTLQ_SEND_MSG_TO_CP,
> > + .chnl_opcode = VIRTCHNL2_OP_RESET_VF,
> > + };
> > +
> > + libie_ctlq_send(adapter->asq, 1);
> > + }
> > +}
> Compared to the old call, which routed through idpf_send_mb_msg() and
> first ran idpf_is_reset_detected() (via the arq registers) to bail out
> early if the mailbox was gone, this new helper unconditionally
> dereferences adapter->asq and immediately calls idpf_mb_clean() which
> takes &ctlq->lock.
> Can adapter->asq be NULL here?
> idpf_deinit_dflt_mbx() sets asq = NULL after libie_ctlq_xn_deinit(),
> and idpf_init_dflt_mbx() can fail with asq/arq still NULL if
> libie_find_ctlq() returns NULL. If a hard reset flow is entered on
> such an error path, ctlq->lock would fault.
> Also, is bypassing the xn manager and reaching directly into libie
> internals (ctlq->tx_msg[ctlq->next_to_use], ctlq->lock, and
> libie_ctlq_send()) intentional? This is the only send path in the
> refactored driver that pokes libie state directly, and it is fragile
> against future changes to how tx_msg[] slots are allocated or
> lifecycled after a forced idpf_mb_clean().
> Finally, is it safe to writel() to the mailbox tail via
> libie_ctlq_send() when hardware is already in reset? The old
> idpf_is_reset_detected() gate specifically avoided that.
VF reset is done only in one flow, where it is performed only
if no other reset is going on.
[...]
>
> > +int idpf_send_mb_msg_kfree(struct idpf_adapter *adapter,
> > + struct libie_ctlq_xn_send_params *xn_params,
> > + void *send_buf, size_t send_buf_size)
> > +{
> > + int err = idpf_send_mb_msg(adapter, xn_params, send_buf, send_buf_size);
> > +
> > + if (libie_cp_can_send_onstack(send_buf_size))
> > + kfree(send_buf);
> > +
> > + return err;
> > +}
> Is the stack-vs-heap discrimination here safe as a long-term invariant?
> The reset-detected branch decides to kfree(send_buf) based purely on
> libie_cp_can_send_onstack(send_buf_size), and rel_tx_buf is set to
> kfree unconditionally so libie_ctlq_xn_send() will also invoke kfree
> on the send buffer when the send fails and the size crosses
> LIBIE_CP_TX_COPYBREAK (128 bytes).
> Many call sites pass the address of a stack-allocated struct:
> struct virtchnl2_get_capabilities caps = {};
> ... idpf_send_mb_msg(adapter, &xn_params, &caps, sizeof(caps));
> struct virtchnl2_vport_stats stats_msg = {};
> ... idpf_send_mb_msg(np->adapter, &xn_params, &stats_msg,
> sizeof(stats_msg));
> virtchnl2_vport_stats is exactly 128 bytes today, so it sits right at
> the LIBIE_CP_TX_COPYBREAK boundary. Adding one field to it, or any
> similar stack-passed structure growing past 128 bytes, would silently
> cause kfree() to be called on a stack address in the reset-detected
> or send-failure path, corrupting the slab allocator.
> Would a BUILD_BUG_ON at each stack-based call site, or a typed helper
> that only accepts on-stack buffers up to LIBIE_CP_TX_COPYBREAK, be
> worth adding so this constraint is enforced at compile time rather
> than by convention?
I would very much prefer to go the callback route right away, like in ixd,
but the diff would be even more unmanageable then.
[...]
>
> > @@ -1586,33 +1168,29 @@ int idpf_send_create_vport_msg(struct idpf_adapter *adapter,
> > }
> >
> > if (!adapter->vport_params_recvd[idx]) {
> > - adapter->vport_params_recvd[idx] = kzalloc(IDPF_CTLQ_MAX_BUF_LEN,
> > - GFP_KERNEL);
> > + adapter->vport_params_recvd[idx] =
> > + kzalloc(LIBIE_CTLQ_MAX_BUF_LEN, GFP_KERNEL);
> > if (!adapter->vport_params_recvd[idx]) {
> > err = -ENOMEM;
> > goto rel_buf;
> > }
> > }
> >
> > - xn_params.vc_op = VIRTCHNL2_OP_CREATE_VPORT;
> > - xn_params.send_buf.iov_base = vport_msg;
> > - xn_params.send_buf.iov_len = buf_size;
> > - xn_params.recv_buf.iov_base = adapter->vport_params_recvd[idx];
> > - xn_params.recv_buf.iov_len = IDPF_CTLQ_MAX_BUF_LEN;
> > - xn_params.timeout_ms = IDPF_VC_XN_DEFAULT_TIMEOUT_MSEC;
> > - reply_sz = idpf_vc_xn_exec(adapter, &xn_params);
> > - if (reply_sz < 0) {
> > - err = reply_sz;
> > - goto free_vport_params;
> > + err = idpf_send_mb_msg_kfree(adapter, &xn_params, vport_msg,
> > + sizeof(*vport_msg));
> > + if (err) {
> > + kfree(adapter->vport_params_recvd[idx]);
> > + adapter->vport_params_recvd[idx] = NULL;
> > + return err;
> > }
> >
> > - kfree(vport_msg);
> > + memcpy(adapter->vport_params_recvd[idx], xn_params.recv_mem.iov_base,
> > + xn_params.recv_mem.iov_len);
> Is the memcpy() length here guaranteed to be bounded?
> adapter->vport_params_recvd[idx] is a fixed LIBIE_CTLQ_MAX_BUF_LEN
> allocation, but the copy length is taken directly from
> xn_params.recv_mem.iov_len without a defensive cap. This function
libie_cp does cap the iov_len at LIBIE_CTLQ_MAX_BUF_LEN in
libie_ctlq_fill_rx_msg(), so this is fine.
> relies on libie's Rx path always delivering iov_len <=
> LIBIE_CTLQ_MAX_BUF_LEN. If that invariant ever changes (larger RX
> buffers, multi-descriptor delivery, or a missing bounds check against
> the descriptor's data_len in libie), this becomes an unchecked heap
> overflow driven by CP firmware.
> Other call sites in the same patch use bounded copies, for example
> idpf_send_get_caps_msg():
> memcpy(&adapter->caps, xn_params.recv_mem.iov_base,
> sizeof(adapter->caps));
> Would using min_t(size_t, LIBIE_CTLQ_MAX_BUF_LEN,
> xn_params.recv_mem.iov_len) match the pattern used elsewhere in this
> file?
>
^ 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