* [PATCH v2 bpf 2/6] selftest: bpf: Add test for bpf_tcp_sock() and RAW socket.
From: Kuniyuki Iwashima @ 2026-05-04 21:04 UTC (permalink / raw)
To: Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi
Cc: John Fastabend, Stanislav Fomichev, Song Liu, Yonghong Song,
Jiri Olsa, Eric Dumazet, Kuniyuki Iwashima, Kuniyuki Iwashima,
bpf, netdev
In-Reply-To: <20260504210610.180150-1-kuniyu@google.com>
Let's extend sockopt_sk.c to cover bpf_tcp_sock() for the
wrong socket type.
Before:
# ./test_progs -t sockopt_sk
[ 151.948613] ==================================================================
[ 151.951376] BUG: KASAN: slab-out-of-bounds in sol_tcp_sockopt+0xc7/0x8e0
[ 151.954159] Read of size 8 at addr ffff88801083d760 by task test_progs/1259
...
run_test:FAIL:getsetsockopt unexpected error: -1 (errno 0)
#427 sockopt_sk:FAIL
After:
#427 sockopt_sk:OK
While at it, missing free() is fixed up.
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
v2: Inverted if (err) to if (!err)
---
.../selftests/bpf/prog_tests/sockopt_sk.c | 18 +++++++++++++++++-
tools/testing/selftests/bpf/progs/sockopt_sk.c | 16 ++++++++++++++++
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
index 53637431ec5d..5fd33ad2eaaf 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
@@ -190,7 +190,7 @@ static int getsetsockopt(void)
fd = socket(AF_NETLINK, SOCK_RAW, 0);
if (fd < 0) {
log_err("Failed to create AF_NETLINK socket");
- return -1;
+ goto err;
}
buf.u32 = 1;
@@ -211,6 +211,22 @@ static int getsetsockopt(void)
}
ASSERT_EQ(optlen, 8, "Unexpected NETLINK_LIST_MEMBERSHIPS value");
+ /* Trick bpf_tcp_sock() with IPPROTO_TCP */
+ close(fd);
+ fd = socket(AF_INET, SOCK_RAW, IPPROTO_TCP);
+ if (fd < 0) {
+ log_err("Failed to create RAW socket");
+ goto err;
+ }
+
+ optlen = 20;
+ errno = 0;
+ err = setsockopt(fd, SOL_TCP, TCP_SAVED_SYN, &buf, optlen);
+ if (!err) {
+ log_err("Unexpected setsockopt(TCP_SAVED_SYN)");
+ goto err;
+ }
+
free(big_buf);
close(fd);
return 0;
diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
index cb990a7d3d45..5e0b27e7855c 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
@@ -149,6 +149,20 @@ int _setsockopt(struct bpf_sockopt *ctx)
if (sk && sk->family == AF_NETLINK)
goto out;
+ if (sk && sk->family == AF_INET && sk->type == SOCK_RAW) {
+ struct bpf_tcp_sock *tp = bpf_tcp_sock(sk);
+
+ if (tp) {
+ char saved_syn[60];
+
+ bpf_getsockopt(sk, SOL_TCP, TCP_SAVED_SYN,
+ &saved_syn, sizeof(saved_syn));
+ goto consumed;
+ }
+
+ goto out;
+ }
+
/* Make sure bpf_get_netns_cookie is callable.
*/
if (bpf_get_netns_cookie(NULL) == 0)
@@ -224,6 +238,8 @@ int _setsockopt(struct bpf_sockopt *ctx)
return 0; /* couldn't get sk storage */
storage->val = optval[0];
+
+consumed:
ctx->optlen = -1; /* BPF has consumed this option, don't call kernel
* setsockopt handler.
*/
--
2.54.0.545.g6539524ca2-goog
^ permalink raw reply related
* [PATCH v2 bpf 3/6] mptcp: bpf: fix type confusion in bpf_mptcp_sock_from_subflow()
From: Kuniyuki Iwashima @ 2026-05-04 21:04 UTC (permalink / raw)
To: Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi
Cc: John Fastabend, Stanislav Fomichev, Song Liu, Yonghong Song,
Jiri Olsa, Eric Dumazet, Kuniyuki Iwashima, Kuniyuki Iwashima,
bpf, netdev, Matthieu Baerts (NGI0)
In-Reply-To: <20260504210610.180150-1-kuniyu@google.com>
From: "Matthieu Baerts (NGI0)" <matttbe@kernel.org>
bpf_mptcp_sock_from_subflow() only checks if sk->sk_protocol is
IPPROTO_TCP, but RAW socket can bypass it:
socket(AF_INET, SOCK_RAW, IPPROTO_TCP)
In this case, it would NOT be valid to call sk_is_mptcp() which will
assume sk is a pointer to a struct tcp_sock, and wrongly checks for:
tcp_sk(sk)->is_mptcp.
Fixes: 3bc253c2e652 ("bpf: Add bpf_skc_to_mptcp_sock_proto")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Reviewed-by: Kuniyuki Iwashima <kuniyu@google.com>
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
v1: https://lore.kernel.org/mptcp/20260430-mptcp-bpf-mptcp-sock-type-v1-1-d2ed5cda7da9@kernel.org/
---
net/mptcp/bpf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 8a16672b94e2..4cc16cbeb328 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -14,7 +14,7 @@
struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
{
- if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
+ if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk))
return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
return NULL;
--
2.54.0.545.g6539524ca2-goog
^ permalink raw reply related
* [PATCH v2 bpf 4/6] bpf: tcp: Fix type confusion in bpf_skc_to_tcp_sock().
From: Kuniyuki Iwashima @ 2026-05-04 21:04 UTC (permalink / raw)
To: Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi
Cc: John Fastabend, Stanislav Fomichev, Song Liu, Yonghong Song,
Jiri Olsa, Eric Dumazet, Kuniyuki Iwashima, Kuniyuki Iwashima,
bpf, netdev
In-Reply-To: <20260504210610.180150-1-kuniyu@google.com>
bpf_skc_to_tcp_sock() only checks if sk->sk_protocol is
IPPROTO_TCP, but RAW socket can bypass it:
socket(AF_INET, SOCK_RAW, IPPROTO_TCP)
Let's use sk_is_tcp().
Fixes: 478cfbdf5f13 ("bpf: Add bpf_skc_to_{tcp, tcp_timewait, tcp_request}_sock() helpers")
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
net/core/filter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index cd88633f8dc1..7d945dc2cb92 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11963,7 +11963,7 @@ const struct bpf_func_proto bpf_skc_to_tcp6_sock_proto = {
BPF_CALL_1(bpf_skc_to_tcp_sock, struct sock *, sk)
{
- if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP)
+ if (sk && sk_fullsock(sk) && sk_is_tcp(sk))
return (unsigned long)sk;
return (unsigned long)NULL;
--
2.54.0.545.g6539524ca2-goog
^ permalink raw reply related
* [PATCH v2 bpf 5/6] bpf: tcp: Fix type confusion in bpf_skc_to_tcp6_sock().
From: Kuniyuki Iwashima @ 2026-05-04 21:04 UTC (permalink / raw)
To: Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi
Cc: John Fastabend, Stanislav Fomichev, Song Liu, Yonghong Song,
Jiri Olsa, Eric Dumazet, Kuniyuki Iwashima, Kuniyuki Iwashima,
bpf, netdev
In-Reply-To: <20260504210610.180150-1-kuniyu@google.com>
bpf_skc_to_tcp6_sock() only checks if sk->sk_protocol is IPPROTO_TCP
and sk->sk_family is AF_INET6, but RAW socket can bypass it:
socket(AF_INET6, SOCK_RAW, IPPROTO_TCP)
Let's check sk->sk_type too.
Fixes: af7ec1383361 ("bpf: Add bpf_skc_to_tcp6_sock() helper")
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
net/core/filter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 7d945dc2cb92..684922efd481 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11947,7 +11947,7 @@ BPF_CALL_1(bpf_skc_to_tcp6_sock, struct sock *, sk)
*/
BTF_TYPE_EMIT(struct tcp6_sock);
if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP &&
- sk->sk_family == AF_INET6)
+ sk->sk_type == SOCK_STREAM && sk->sk_family == AF_INET6)
return (unsigned long)sk;
return (unsigned long)NULL;
--
2.54.0.545.g6539524ca2-goog
^ permalink raw reply related
* [PATCH v2 bpf 6/6] bpf: tcp: Fix type confusion in sol_tcp_sockopt().
From: Kuniyuki Iwashima @ 2026-05-04 21:04 UTC (permalink / raw)
To: Martin KaFai Lau, Daniel Borkmann, Alexei Starovoitov,
Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi
Cc: John Fastabend, Stanislav Fomichev, Song Liu, Yonghong Song,
Jiri Olsa, Eric Dumazet, Kuniyuki Iwashima, Kuniyuki Iwashima,
bpf, netdev
In-Reply-To: <20260504210610.180150-1-kuniyu@google.com>
sol_tcp_sockopt() only checks if sk->sk_protocol is IPPROTO_TCP,
but RAW socket can bypass it:
socket(AF_INET, SOCK_RAW, IPPROTO_TCP)
Let's use sk_is_tcp().
Note that initially sol_tcp_sockopt() checked sk->sk_prot->setsockopt.
Fixes: 2ab42c7b871f ("bpf: Check the protocol of a sock to agree the calls to bpf_setsockopt().")
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
net/core/filter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 684922efd481..ef0877eefaa7 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5481,7 +5481,7 @@ static int sol_tcp_sockopt(struct sock *sk, int optname,
char *optval, int *optlen,
bool getopt)
{
- if (sk->sk_protocol != IPPROTO_TCP)
+ if (!sk_is_tcp(sk))
return -EINVAL;
switch (optname) {
--
2.54.0.545.g6539524ca2-goog
^ permalink raw reply related
* Re: [PATCH 08/11] vfio: selftests: Add dev_dbg
From: David Matlack @ 2026-05-04 21:15 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Alex Williamson, kvm, Leon Romanovsky, linux-kselftest,
linux-rdma, Mark Bloch, netdev, Saeed Mahameed, Shuah Khan,
Tariq Toukan, patches
In-Reply-To: <8-v1-dc5fa250ca1d+3213-mlx5st_jgg@nvidia.com>
On 2026-04-30 09:08 PM, Jason Gunthorpe wrote:
> Enable it with a #define DEBUG at the top of the file. Allows leaving
> behind debugging prints that are useful in case future changes are
> required.
>
> Assisted-by: Claude:claude-opus-4.6
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> .../selftests/vfio/lib/include/libvfio/vfio_pci_device.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h b/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
> index bb4525abd01a22..2d587b988c09fa 100644
> --- a/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
> +++ b/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
> @@ -38,6 +38,12 @@ struct vfio_pci_device {
> #define dev_info(_dev, _fmt, ...) printf("%s: " _fmt, (_dev)->bdf, ##__VA_ARGS__)
> #define dev_err(_dev, _fmt, ...) fprintf(stderr, "%s: " _fmt, (_dev)->bdf, ##__VA_ARGS__)
>
> +#ifdef DEBUG
> +#define dev_dbg dev_info
> +#else
> +#define dev_dbg(_dev, _fmt, ...) do { } while (0)
Can you add something to make sure the format strings are still
validated by the compiler even if DEBUG is not defined? (since it
will almost never be defined). e.g.
diff --git a/tools/testing/selftests/vfio/lib/include/libvfio/assert.h b/tools/testing/selftests/vfio/lib/include/libvfio/assert.h
index f4ebd122d9b6..406c430ef28d 100644
--- a/tools/testing/selftests/vfio/lib/include/libvfio/assert.h
+++ b/tools/testing/selftests/vfio/lib/include/libvfio/assert.h
@@ -51,4 +51,9 @@
VFIO_ASSERT_EQ(__ret, 0, "ioctl(%s, %s, %s) returned %d\n", #_fd, #_op, #_arg, __ret); \
} while (0)
+ __attribute__((__format__(__printf__, 1, 2)))
+static inline void check_format_string(const char *fmt, ...)
+{
+}
+
#endif /* SELFTESTS_VFIO_LIB_INCLUDE_LIBVFIO_ASSERT_H */
diff --git a/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h b/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
index 2d587b988c09..3abfa6ff481c 100644
--- a/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
+++ b/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
@@ -39,9 +39,9 @@ struct vfio_pci_device {
#define dev_err(_dev, _fmt, ...) fprintf(stderr, "%s: " _fmt, (_dev)->bdf, ##__VA_ARGS__)
#ifdef DEBUG
-#define dev_dbg dev_info
+#define dev_dbg(_dev, _fmt, ...) dev_info
#else
-#define dev_dbg(_dev, _fmt, ...) do { } while (0)
+#define dev_dbg(_dev, _fmt, ...) check_format_string(_fmt, ##__VA_ARGS__)
#endif
struct vfio_pci_device *vfio_pci_device_init(const char *bdf, struct iommu *iommu);
^ permalink raw reply related
* [PATCH net-next v2 1/2] net: page_pool: support dumping pps of a specific ifindex via Netlink
From: Jakub Kicinski @ 2026-05-04 21:43 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
shuah, matttbe, hawk, linux-kselftest, maxime.chevallier,
Jakub Kicinski
NIPA tries to make sure that HW tests don't modify system state.
It saves the state of page pools, too. Now that I write this commit
message I realize that this is impractical since page pool IDs and
state will get legitimately changed by the tests. But I already
spent a couple of hours implementing the filtering, so..
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
- adjust ynltool to pass empty req pointer now
v1: https://lore.kernel.org/20260319035649.2396137-1-kuba@kernel.org
---
Documentation/netlink/specs/netdev.yaml | 6 ++++
net/core/netdev-genl-gen.c | 30 ++++++++++++----
net/core/page_pool_user.c | 47 +++++++++++++++++++++++--
tools/net/ynl/ynltool/page-pool.c | 6 ++--
4 files changed, 78 insertions(+), 11 deletions(-)
diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index b93beb247a11..a1f4c5a561e9 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -649,6 +649,9 @@ doc: >-
- dmabuf
- io-uring
dump:
+ request:
+ attributes:
+ - ifindex
reply: *pp-reply
config-cond: page-pool
-
@@ -692,6 +695,9 @@ doc: >-
- recycle-ring-full
- recycle-released-refcnt
dump:
+ request:
+ attributes:
+ - info
reply: *pp-stats-reply
config-cond: page-pool-stats
-
diff --git a/net/core/netdev-genl-gen.c b/net/core/netdev-genl-gen.c
index 81aecb5d3bc5..f605ed1e9872 100644
--- a/net/core/netdev-genl-gen.c
+++ b/net/core/netdev-genl-gen.c
@@ -54,6 +54,11 @@ static const struct nla_policy netdev_dev_get_nl_policy[NETDEV_A_DEV_IFINDEX + 1
static const struct nla_policy netdev_page_pool_get_nl_policy[NETDEV_A_PAGE_POOL_ID + 1] = {
[NETDEV_A_PAGE_POOL_ID] = NLA_POLICY_FULL_RANGE(NLA_UINT, &netdev_a_page_pool_id_range),
};
+
+/* NETDEV_CMD_PAGE_POOL_GET - dump */
+static const struct nla_policy netdev_page_pool_get_dump_nl_policy[NETDEV_A_PAGE_POOL_IFINDEX + 1] = {
+ [NETDEV_A_PAGE_POOL_IFINDEX] = NLA_POLICY_FULL_RANGE(NLA_U32, &netdev_a_page_pool_ifindex_range),
+};
#endif /* CONFIG_PAGE_POOL */
/* NETDEV_CMD_PAGE_POOL_STATS_GET - do */
@@ -61,6 +66,15 @@ static const struct nla_policy netdev_page_pool_get_nl_policy[NETDEV_A_PAGE_POOL
static const struct nla_policy netdev_page_pool_stats_get_nl_policy[NETDEV_A_PAGE_POOL_STATS_INFO + 1] = {
[NETDEV_A_PAGE_POOL_STATS_INFO] = NLA_POLICY_NESTED(netdev_page_pool_info_nl_policy),
};
+
+/* NETDEV_CMD_PAGE_POOL_STATS_GET - dump */
+static const struct nla_policy netdev_page_pool_stats_get_dump_info_nl_policy[NETDEV_A_PAGE_POOL_IFINDEX + 1] = {
+ [NETDEV_A_PAGE_POOL_IFINDEX] = NLA_POLICY_FULL_RANGE(NLA_U32, &netdev_a_page_pool_ifindex_range),
+};
+
+static const struct nla_policy netdev_page_pool_stats_get_dump_nl_policy[NETDEV_A_PAGE_POOL_STATS_INFO + 1] = {
+ [NETDEV_A_PAGE_POOL_STATS_INFO] = NLA_POLICY_NESTED(netdev_page_pool_stats_get_dump_info_nl_policy),
+};
#endif /* CONFIG_PAGE_POOL_STATS */
/* NETDEV_CMD_QUEUE_GET - do */
@@ -143,9 +157,11 @@ static const struct genl_split_ops netdev_nl_ops[] = {
.flags = GENL_CMD_CAP_DO,
},
{
- .cmd = NETDEV_CMD_PAGE_POOL_GET,
- .dumpit = netdev_nl_page_pool_get_dumpit,
- .flags = GENL_CMD_CAP_DUMP,
+ .cmd = NETDEV_CMD_PAGE_POOL_GET,
+ .dumpit = netdev_nl_page_pool_get_dumpit,
+ .policy = netdev_page_pool_get_dump_nl_policy,
+ .maxattr = NETDEV_A_PAGE_POOL_IFINDEX,
+ .flags = GENL_CMD_CAP_DUMP,
},
#endif /* CONFIG_PAGE_POOL */
#ifdef CONFIG_PAGE_POOL_STATS
@@ -157,9 +173,11 @@ static const struct genl_split_ops netdev_nl_ops[] = {
.flags = GENL_CMD_CAP_DO,
},
{
- .cmd = NETDEV_CMD_PAGE_POOL_STATS_GET,
- .dumpit = netdev_nl_page_pool_stats_get_dumpit,
- .flags = GENL_CMD_CAP_DUMP,
+ .cmd = NETDEV_CMD_PAGE_POOL_STATS_GET,
+ .dumpit = netdev_nl_page_pool_stats_get_dumpit,
+ .policy = netdev_page_pool_stats_get_dump_nl_policy,
+ .maxattr = NETDEV_A_PAGE_POOL_STATS_INFO,
+ .flags = GENL_CMD_CAP_DUMP,
},
#endif /* CONFIG_PAGE_POOL_STATS */
{
diff --git a/net/core/page_pool_user.c b/net/core/page_pool_user.c
index ee5060d8eec0..01509d1b3cba 100644
--- a/net/core/page_pool_user.c
+++ b/net/core/page_pool_user.c
@@ -79,7 +79,7 @@ struct page_pool_dump_cb {
static int
netdev_nl_page_pool_get_dump(struct sk_buff *skb, struct netlink_callback *cb,
- pp_nl_fill_cb fill)
+ pp_nl_fill_cb fill, struct nlattr *ifindex_attr)
{
struct page_pool_dump_cb *state = (void *)cb->ctx;
const struct genl_info *info = genl_info_dump(cb);
@@ -88,9 +88,17 @@ netdev_nl_page_pool_get_dump(struct sk_buff *skb, struct netlink_callback *cb,
struct page_pool *pool;
int err = 0;
+ if (ifindex_attr)
+ state->ifindex = nla_get_u32(ifindex_attr);
+
rtnl_lock();
mutex_lock(&page_pools_lock);
for_each_netdev_dump(net, netdev, state->ifindex) {
+ /* Either the provided ifindex doesn't exist or done dumping */
+ if (ifindex_attr &&
+ netdev->ifindex != nla_get_u32(ifindex_attr))
+ break;
+
hlist_for_each_entry(pool, &netdev->page_pools, user.list) {
if (state->pp_id && state->pp_id < pool->user.id)
continue;
@@ -206,10 +214,40 @@ int netdev_nl_page_pool_stats_get_doit(struct sk_buff *skb,
return netdev_nl_page_pool_get_do(info, id, page_pool_nl_stats_fill);
}
+static const struct netlink_range_validation page_pool_ifindex_range = {
+ .min = 1ULL,
+ .max = S32_MAX,
+};
+
+static const struct nla_policy
+page_pool_stat_info_policy[NETDEV_A_PAGE_POOL_IFINDEX + 1] = {
+ [NETDEV_A_PAGE_POOL_IFINDEX] =
+ NLA_POLICY_FULL_RANGE(NLA_U32, &page_pool_ifindex_range),
+};
+
int netdev_nl_page_pool_stats_get_dumpit(struct sk_buff *skb,
struct netlink_callback *cb)
{
- return netdev_nl_page_pool_get_dump(skb, cb, page_pool_nl_stats_fill);
+ struct nlattr *tb[ARRAY_SIZE(page_pool_stat_info_policy)];
+ const struct genl_info *info = genl_info_dump(cb);
+ struct nlattr *ifindex_attr = NULL;
+
+ if (info->attrs[NETDEV_A_PAGE_POOL_STATS_INFO]) {
+ struct nlattr *nest;
+ int err;
+
+ nest = info->attrs[NETDEV_A_PAGE_POOL_STATS_INFO];
+ err = nla_parse_nested(tb, ARRAY_SIZE(tb) - 1, nest,
+ page_pool_stat_info_policy,
+ info->extack);
+ if (err)
+ return err;
+
+ ifindex_attr = tb[NETDEV_A_PAGE_POOL_IFINDEX];
+ }
+
+ return netdev_nl_page_pool_get_dump(skb, cb, page_pool_nl_stats_fill,
+ ifindex_attr);
}
static int
@@ -305,7 +343,10 @@ int netdev_nl_page_pool_get_doit(struct sk_buff *skb, struct genl_info *info)
int netdev_nl_page_pool_get_dumpit(struct sk_buff *skb,
struct netlink_callback *cb)
{
- return netdev_nl_page_pool_get_dump(skb, cb, page_pool_nl_fill);
+ const struct genl_info *info = genl_info_dump(cb);
+
+ return netdev_nl_page_pool_get_dump(skb, cb, page_pool_nl_fill,
+ info->attrs[NETDEV_A_PAGE_POOL_IFINDEX]);
}
int page_pool_list(struct page_pool *pool)
diff --git a/tools/net/ynl/ynltool/page-pool.c b/tools/net/ynl/ynltool/page-pool.c
index 4b24492abab7..9487eda6b3aa 100644
--- a/tools/net/ynl/ynltool/page-pool.c
+++ b/tools/net/ynl/ynltool/page-pool.c
@@ -327,7 +327,9 @@ static void aggregate_device_stats(struct pp_stats_array *a,
static int do_stats(int argc, char **argv)
{
+ struct netdev_page_pool_stats_get_req_dump pp_stat_req = {};
struct netdev_page_pool_stats_get_list *pp_stats;
+ struct netdev_page_pool_get_req_dump pp_req = {};
struct netdev_page_pool_get_list *pools;
enum {
GROUP_BY_DEVICE,
@@ -374,14 +376,14 @@ static int do_stats(int argc, char **argv)
return -1;
}
- pools = netdev_page_pool_get_dump(ys);
+ pools = netdev_page_pool_get_dump(ys, &pp_req);
if (!pools) {
p_err("failed to get page pools: %s", ys->err.msg);
ret = -1;
goto exit_close;
}
- pp_stats = netdev_page_pool_stats_get_dump(ys);
+ pp_stats = netdev_page_pool_stats_get_dump(ys, &pp_stat_req);
if (!pp_stats) {
p_err("failed to get page pool stats: %s", ys->err.msg);
ret = -1;
--
2.54.0
^ permalink raw reply related
* [PATCH net-next v2 2/2] selftests: net: add tests for filtered dumps of page pool
From: Jakub Kicinski @ 2026-05-04 21:43 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, andrew+netdev, horms, donald.hunter,
shuah, matttbe, hawk, linux-kselftest, maxime.chevallier,
Jakub Kicinski
In-Reply-To: <20260504214336.613107-1-kuba@kernel.org>
Add tests for page pool dumps of a specific ifindex.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
- add CONFIG_PAGE_POOL_STATS=y to the config
v1: https://lore.kernel.org/20260319035649.2396137-2-kuba@kernel.org
---
tools/testing/selftests/net/config | 1 +
tools/testing/selftests/net/nl_netdev.py | 119 ++++++++++++++++++++++-
2 files changed, 118 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/config b/tools/testing/selftests/net/config
index 94d722770420..d07c5ac5cab7 100644
--- a/tools/testing/selftests/net/config
+++ b/tools/testing/selftests/net/config
@@ -117,6 +117,7 @@ CONFIG_OPENVSWITCH=m
CONFIG_OPENVSWITCH_GENEVE=m
CONFIG_OPENVSWITCH_GRE=m
CONFIG_OPENVSWITCH_VXLAN=m
+CONFIG_PAGE_POOL_STATS=y
CONFIG_PROC_SYSCTL=y
CONFIG_PSAMPLE=m
CONFIG_RPS=y
diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
index eff55c64a012..ceb44c8e1fec 100755
--- a/tools/testing/selftests/net/nl_netdev.py
+++ b/tools/testing/selftests/net/nl_netdev.py
@@ -9,7 +9,7 @@ import errno
from os import system
from lib.py import ksft_run, ksft_exit
from lib.py import ksft_eq, ksft_ge, ksft_ne, ksft_raises, ksft_busy_wait
-from lib.py import NetdevFamily, NetdevSimDev, NlError, ip
+from lib.py import NetdevFamily, NetdevSimDev, NlError, defer, ip
def empty_check(nf) -> None:
@@ -255,6 +255,117 @@ from lib.py import NetdevFamily, NetdevSimDev, NlError, ip
nsim.dfs_write("pp_hold", "y")
+def page_pool_dump_ifindex(nf) -> None:
+ """Test page pool dump filtering by ifindex."""
+ nsimdev1 = NetdevSimDev(queue_count=3)
+ rm_nsim1 = defer(nsimdev1.remove)
+ nsimdev2 = NetdevSimDev(queue_count=5)
+ defer(nsimdev2.remove)
+
+ nsim1 = nsimdev1.nsims[0]
+ nsim2 = nsimdev2.nsims[0]
+
+ ip(f"link set dev {nsim1.ifname} up")
+ ip(f"link set dev {nsim2.ifname} up")
+
+ # Unfiltered dump should have pools from both devices
+ all_pp = nf.page_pool_get({}, dump=True)
+ pp1_all = [pp for pp in all_pp
+ if pp.get("ifindex") == nsim1.ifindex]
+ pp2_all = [pp for pp in all_pp
+ if pp.get("ifindex") == nsim2.ifindex]
+ ksft_ge(len(pp1_all), 1)
+ ksft_ge(len(pp2_all), 1)
+
+ # Filtered dump should only return pools for that device
+ pp1_flt = nf.page_pool_get({'ifindex': nsim1.ifindex}, dump=True)
+ ksft_eq(pp1_flt, pp1_all)
+
+ pp2_flt = nf.page_pool_get({'ifindex': nsim2.ifindex}, dump=True)
+ ksft_eq(pp2_flt, pp2_all)
+
+ # Non-existent ifindex should return empty dump
+ pp_none = nf.page_pool_get({'ifindex': 12345678}, dump=True)
+ ksft_eq(len(pp_none), 0)
+
+ # Device down - no pools for that ifindex
+ ip(f"link set dev {nsim1.ifname} down")
+ pp1_down = nf.page_pool_get({'ifindex': nsim1.ifindex}, dump=True)
+ ksft_eq(len(pp1_down), 0)
+
+ # Remove device, dump by its old ifindex should return empty
+ old_ifindex = nsim1.ifindex
+ rm_nsim1.exec()
+ pp1_gone = nf.page_pool_get({'ifindex': old_ifindex}, dump=True)
+ ksft_eq(len(pp1_gone), 0)
+
+
+def page_pool_ifindex_leak_check(nf) -> None:
+ """Test that zombie page pools don't show up under the original ifindex."""
+ nsimdev = NetdevSimDev()
+ rm_nsim = defer(nsimdev.remove)
+ nsim = nsimdev.nsims[0]
+
+ ip(f"link set dev {nsim.ifname} up")
+ nsim.dfs_write("pp_hold", "y")
+
+ pp_up = nf.page_pool_get({'ifindex': nsim.ifindex}, dump=True)
+ ksft_ge(len(pp_up), 1)
+
+ # Remove device with leaked page - pool becomes zombie (orphaned to lo)
+ old_ifindex = nsim.ifindex
+ rm_nsim.exec()
+
+ # Zombie pool should NOT appear under the original device
+ pp_down = nf.page_pool_get({'ifindex': old_ifindex}, dump=True)
+ ksft_eq(len(pp_down), 0)
+
+ # But it should appear in an unfiltered dump (under loopback)
+ pp_all = nf.page_pool_get({}, dump=True)
+ orphans = [pp for pp in pp_all
+ if "detach-time" in pp and "ifindex" not in pp]
+ ksft_ge(len(orphans), 1)
+
+
+def page_pool_stats_ifindex_check(nf) -> None:
+ """Test page pool stats dump filtering by ifindex."""
+ nsimdev1 = NetdevSimDev(queue_count=3)
+ defer(nsimdev1.remove)
+ nsimdev2 = NetdevSimDev(queue_count=5)
+ defer(nsimdev2.remove)
+
+ nsim1 = nsimdev1.nsims[0]
+ nsim2 = nsimdev2.nsims[0]
+
+ ip(f"link set dev {nsim1.ifname} up")
+ ip(f"link set dev {nsim2.ifname} up")
+
+ # Unfiltered stats dump
+ all_stats = nf.page_pool_stats_get({}, dump=True)
+ s1_all = [s for s in all_stats
+ if s.get("info", {}).get("ifindex") == nsim1.ifindex]
+ s2_all = [s for s in all_stats
+ if s.get("info", {}).get("ifindex") == nsim2.ifindex]
+ ksft_ge(len(s1_all), 1)
+ ksft_ge(len(s2_all), 1)
+
+ # Filtered stats dump
+ s1_flt = nf.page_pool_stats_get({'info': {'ifindex': nsim1.ifindex}},
+ dump=True)
+ ksft_eq(s1_flt, s1_all)
+
+ # Non-existent ifindex should return empty
+ s_none = nf.page_pool_stats_get({'info': {'ifindex': 12345678}}, dump=True)
+ ksft_eq(len(s_none), 0)
+
+ # info.id should be rejected for stats dump
+ with ksft_raises(NlError) as cm:
+ nf.page_pool_stats_get({'info': {'id': s1_all[0]['info']['id']}},
+ dump=True)
+ ksft_eq(cm.exception.nl_msg.error, -errno.EINVAL)
+ ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.info.id')
+
+
def main() -> None:
""" Ksft boiler plate main """
nf = NetdevFamily()
@@ -265,7 +376,11 @@ from lib.py import NetdevFamily, NetdevSimDev, NlError, ip
napi_set_threaded,
dev_set_threaded,
nsim_rxq_reset_down,
- page_pool_check],
+ page_pool_check,
+ page_pool_dump_ifindex,
+ page_pool_ifindex_leak_check,
+ page_pool_stats_ifindex_check
+ ],
args=(nf, ))
ksft_exit()
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 1/2] vfio: add dma-buf get_tph callback and DMA_BUF_TPH feature
From: Alex Williamson @ 2026-05-04 21:44 UTC (permalink / raw)
To: Zhiping Zhang
Cc: Jason Gunthorpe, Leon Romanovsky, Bjorn Helgaas, linux-rdma,
linux-pci, netdev, dri-devel, Keith Busch, Yochai Cohen,
Yishai Hadas, alex
In-Reply-To: <20260430200704.352228-2-zhipingz@meta.com>
On Thu, 30 Apr 2026 13:06:56 -0700
Zhiping Zhang <zhipingz@meta.com> wrote:
> Add a dma-buf callback that returns raw TPH metadata from the exporter
> so peer devices can reuse the steering tag and processing hint
> associated with a VFIO-exported buffer.
>
> Add a new VFIO_DEVICE_FEATURE_DMA_BUF_TPH ioctl that takes the fd from
> VFIO_DEVICE_FEATURE_DMA_BUF along with a steering tag and processing
> hint, validates the fd is a vfio-exported dma-buf belonging to this
> device, and stores the TPH values under memory_lock. This keeps the
> existing VFIO_DEVICE_FEATURE_DMA_BUF uAPI completely unchanged.
>
> The user sequences setting TPH on the dma-buf before the importer
> consumes it.
>
> Add an st_width parameter to get_tph() so the exporter can reject
> steering tags that exceed the consumer's supported width (8 vs 16 bit).
> When no TPH metadata was supplied, get_tph() returns -EOPNOTSUPP.
>
> Signed-off-by: Zhiping Zhang <zhipingz@meta.com>
The uAPI is better, but sashiko has some review comments[1] for you.
Please also copy the kvm list for vfio related development. Thanks,
Alex
[1]https://sashiko.dev/#/patchset/20260430200704.352228-1-zhipingz@meta.com
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1534,6 +1534,9 @@ int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
> return vfio_pci_core_feature_token(vdev, flags, arg, argsz);
> case VFIO_DEVICE_FEATURE_DMA_BUF:
> return vfio_pci_core_feature_dma_buf(vdev, flags, arg, argsz);
> + case VFIO_DEVICE_FEATURE_DMA_BUF_TPH:
> + return vfio_pci_core_feature_dma_buf_tph(vdev, flags, arg,
> + argsz);
> default:
> return -ENOTTY;
> }
> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
> @@ -19,6 +19,9 @@ struct vfio_pci_dma_buf {
> u32 nr_ranges;
> struct kref kref;
> struct completion comp;
> + u16 steering_tag;
> + u8 ph;
> + u8 tph_present : 1;
> u8 revoked : 1;
> };
>
> @@ -69,6 +72,22 @@ vfio_pci_dma_buf_map(struct dma_buf_attachment *attachment,
> return ret;
> }
>
> +static int vfio_pci_dma_buf_get_tph(struct dma_buf *dmabuf, u16 *steering_tag,
> + u8 *ph, u8 st_width)
> +{
> + struct vfio_pci_dma_buf *priv = dmabuf->priv;
> +
> + if (!priv->tph_present)
> + return -EOPNOTSUPP;
> +
> + if (st_width < 16 && priv->steering_tag > ((1U << st_width) - 1))
> + return -EINVAL;
> +
> + *steering_tag = priv->steering_tag;
> + *ph = priv->ph;
> + return 0;
> +}
> +
> static void vfio_pci_dma_buf_unmap(struct dma_buf_attachment *attachment,
> struct sg_table *sgt,
> enum dma_data_direction dir)
> @@ -101,6 +120,7 @@ static void vfio_pci_dma_buf_release(struct dma_buf *dmabuf)
>
> static const struct dma_buf_ops vfio_pci_dmabuf_ops = {
> .attach = vfio_pci_dma_buf_attach,
> + .get_tph = vfio_pci_dma_buf_get_tph,
> .map_dma_buf = vfio_pci_dma_buf_map,
> .unmap_dma_buf = vfio_pci_dma_buf_unmap,
> .release = vfio_pci_dma_buf_release,
> @@ -331,6 +351,55 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
> return ret;
> }
>
> +int vfio_pci_core_feature_dma_buf_tph(struct vfio_pci_core_device *vdev,
> + u32 flags,
> + struct vfio_device_feature_dma_buf_tph __user *arg,
> + size_t argsz)
> +{
> + struct vfio_device_feature_dma_buf_tph set_tph;
> + struct vfio_pci_dma_buf *priv;
> + struct dma_buf *dmabuf;
> + int ret;
> +
> + ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
> + sizeof(set_tph));
> + if (ret != 1)
> + return ret;
> +
> + if (copy_from_user(&set_tph, arg, sizeof(set_tph)))
> + return -EFAULT;
> +
> + if (set_tph.reserved)
> + return -EINVAL;
> +
> + dmabuf = dma_buf_get(set_tph.dmabuf_fd);
> + if (IS_ERR(dmabuf))
> + return PTR_ERR(dmabuf);
> +
> + if (dmabuf->ops != &vfio_pci_dmabuf_ops) {
> + ret = -EINVAL;
> + goto out_put;
> + }
> +
> + priv = dmabuf->priv;
> + down_write(&vdev->memory_lock);
> + if (priv->vdev != vdev) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +
> + priv->steering_tag = set_tph.steering_tag;
> + priv->ph = set_tph.ph;
> + priv->tph_present = 1;
> + ret = 0;
> +
> +out_unlock:
> + up_write(&vdev->memory_lock);
> +out_put:
> + dma_buf_put(dmabuf);
> + return ret;
> +}
> +
> void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked)
> {
> struct vfio_pci_dma_buf *priv;
> diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
> --- a/drivers/vfio/pci/vfio_pci_priv.h
> +++ b/drivers/vfio/pci/vfio_pci_priv.h
> @@ -118,6 +118,10 @@ static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
> int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
> struct vfio_device_feature_dma_buf __user *arg,
> size_t argsz);
> +int vfio_pci_core_feature_dma_buf_tph(struct vfio_pci_core_device *vdev,
> + u32 flags,
> + struct vfio_device_feature_dma_buf_tph __user *arg,
> + size_t argsz);
> void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev);
> void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked);
> #else
> @@ -128,6 +132,13 @@ vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
> {
> return -ENOTTY;
> }
> +static inline int
> +vfio_pci_core_feature_dma_buf_tph(struct vfio_pci_core_device *vdev, u32 flags,
> + struct vfio_device_feature_dma_buf_tph __user *arg,
> + size_t argsz)
> +{
> + return -ENOTTY;
> +}
> static inline void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev)
> {
> }
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -113,6 +113,23 @@ struct dma_buf_ops {
> */
> void (*unpin)(struct dma_buf_attachment *attach);
>
> + /**
> + * @get_tph:
> + * @dmabuf: DMA buffer for which to retrieve TPH metadata
> + * @steering_tag: Returns the raw TPH steering tag
> + * @ph: Returns the TPH processing hint
> + * @st_width: Consumer's supported steering tag width in bits (8 or 16)
> + *
> + * Return the TPH (TLP Processing Hints) metadata associated with this
> + * DMA buffer. Exporters that do not provide TPH metadata should return
> + * -EOPNOTSUPP. If the steering tag exceeds @st_width bits, return
> + * -EINVAL.
> + *
> + * This callback is optional.
> + */
> + int (*get_tph)(struct dma_buf *dmabuf, u16 *steering_tag, u8 *ph,
> + u8 st_width);
> +
> /**
> * @map_dma_buf:
> *
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -1534,6 +1534,28 @@ struct vfio_device_feature_dma_buf {
> */
> #define VFIO_DEVICE_FEATURE_MIG_PRECOPY_INFOv2 12
>
> +/**
> + * Upon VFIO_DEVICE_FEATURE_SET associate TPH (TLP Processing Hints) metadata
> + * with a vfio-exported dma-buf. The dma-buf must have been created by
> + * VFIO_DEVICE_FEATURE_DMA_BUF on this device.
> + *
> + * dmabuf_fd is the file descriptor returned by VFIO_DEVICE_FEATURE_DMA_BUF.
> + * steering_tag and ph are the raw TPH values that importing drivers should use
> + * when accessing the buffer.
> + *
> + * The user must set TPH on the dma-buf before the importer consumes it.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +#define VFIO_DEVICE_FEATURE_DMA_BUF_TPH 13
> +
> +struct vfio_device_feature_dma_buf_tph {
> + __s32 dmabuf_fd;
> + __u16 steering_tag;
> + __u8 ph;
> + __u8 reserved;
> +};
> +
> /* -------- API for Type1 VFIO IOMMU -------- */
>
> /**
^ permalink raw reply
* Re: [PATCH 05/11] selftests: Add additional kernel functions to tools/include/
From: David Matlack @ 2026-05-04 21:48 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Alex Williamson, kvm, Leon Romanovsky, linux-kselftest,
linux-rdma, Mark Bloch, netdev, Saeed Mahameed, Shuah Khan,
Tariq Toukan, patches
In-Reply-To: <5-v1-dc5fa250ca1d+3213-mlx5st_jgg@nvidia.com>
On 2026-04-30 09:08 PM, Jason Gunthorpe wrote:
> These are needed by the VFIO mlx5 selftest in the following patches,
> which includes some headers from mlx5 and also needs a few more
> MMIO-related features.
>
> - DECLARE_FLEX_ARRAY in new tools/include/linux/stddef.h (wraps
> existing __DECLARE_FLEX_ARRAY from uapi/linux/stddef.h)
Is this needed? I don't see it used anywhere.
$ git grep DECLARE_FLEX_ARRAY tools/testing/selftests/vfio
^ permalink raw reply
* Re: [PATCH v2 bpf 1/6] bpf: tcp: Fix type confusion in bpf_tcp_sock().
From: bot+bpf-ci @ 2026-05-04 21:50 UTC (permalink / raw)
To: kuniyu, martin.lau, daniel, ast, andrii, eddyz87, memxor
Cc: john.fastabend, sdf, song, yonghong.song, jolsa, edumazet, kuniyu,
kuni1840, bpf, netdev, melotti, ast, andrii, daniel, martin.lau,
eddyz87, yonghong.song, clm, ihor.solodrai
In-Reply-To: <20260504210610.180150-2-kuniyu@google.com>
[-- Attachment #1: Type: text/plain, Size: 3247 bytes --]
> diff --git a/net/core/filter.c b/net/core/filter.c
> index bc96c18df4e0..cd88633f8dc1 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7475,7 +7475,7 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
>
> BPF_CALL_1(bpf_tcp_sock, struct sock *, sk)
> {
> - if (sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP)
> + if (sk_fullsock(sk) && sk_is_tcp(sk))
> return (unsigned long)sk;
>
> return (unsigned long)NULL;
While this patch fixes bpf_tcp_sock(), does sol_tcp_sockopt() remain
vulnerable? Since ctx->sk evaluates to PTR_TO_SOCKET in a CGROUP_SOCKOPT
program and is directly accepted by bpf_setsockopt() and bpf_getsockopt()
via ARG_PTR_TO_BTF_ID_SOCK_COMMON, can a user create a RAW socket with
IPPROTO_TCP and directly pass ctx->sk to bpf_setsockopt(), bypassing
bpf_tcp_sock() entirely?
Looking at sol_tcp_sockopt():
static int sol_tcp_sockopt(struct sock *sk, int optname,
char *optval, int *optlen,
bool getopt)
{
if (sk->sk_protocol != IPPROTO_TCP)
return -EINVAL;
This appears to only check sk->sk_protocol == IPPROTO_TCP without verifying
sk->sk_type == SOCK_STREAM, which would allow the same slab out-of-bounds
access through a different code path.
Additionally, do bpf_skc_to_tcp_sock() and bpf_skc_to_tcp6_sock() suffer
from the same type confusion? They appear to check sk->sk_protocol ==
IPPROTO_TCP without verifying sk->sk_type == SOCK_STREAM.
Could a BPF program pass a RAW socket with IPPROTO_TCP to these helpers
and receive a valid BTF pointer to struct tcp_sock, allowing out-of-bounds
slab reads?
Looking at bpf_skc_to_tcp_sock():
BPF_CALL_1(bpf_skc_to_tcp_sock, struct sock *, sk)
{
if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP)
return (unsigned long)sk;
return (unsigned long)NULL;
}
And bpf_skc_to_tcp6_sock():
BPF_CALL_1(bpf_skc_to_tcp6_sock, struct sock *, sk)
{
/* tcp6_sock type is not generated in dwarf and hence btf,
* trigger an explicit type generation here.
*/
BTF_TYPE_EMIT(struct tcp6_sock);
if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP &&
sk->sk_family == AF_INET6)
return (unsigned long)sk;
Finally, does bpf_mptcp_sock_from_subflow() in net/mptcp/bpf.c also need
a similar update? It checks sk->sk_protocol == IPPROTO_TCP and immediately
evaluates sk_is_mptcp(sk).
Since sk_is_mptcp() indiscriminately casts the socket to struct tcp_sock
and accesses tcp_sk(sk)->is_mptcp, passing a RAW socket could trigger an
immediate out-of-bounds slab read. If this memory happens to be non-zero,
it might proceed to call mptcp_subflow_ctx(sk)->conn, which would read a
garbage pointer from out-of-bounds memory:
struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
{
if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
return NULL;
}
These issues were raised by sashiko-bot@kernel.org in v1:
https://lore.kernel.org/bpf/20260501184452.7F46FC2BCB4@smtp.kernel.org/
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25344293379
^ permalink raw reply
* RE: [EXTERNAL] Re: [PATCH net-next v6 1/6] net: mana: Create separate EQs for each vPort
From: Long Li @ 2026-05-04 21:51 UTC (permalink / raw)
To: Simon Horman
Cc: Konstantin Taranov, kuba@kernel.org, davem@davemloft.net,
pabeni@redhat.com, edumazet@google.com, andrew+netdev@lunn.ch,
jgg@ziepe.ca, leon@kernel.org, Haiyang Zhang, KY Srinivasan,
wei.liu@kernel.org, Dexuan Cui, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20260502150717.281387-1-horms@kernel.org>
> From: 'Simon Horman' <horms@kernel.org>
>
> This is an AI-generated review of your patch. The human sending this email has
> considered the AI review valid, or at least plausible.
> Full review at:
> https://sashiko.d/
> ev%2F&data=05%7C02%7Clongli%40microsoft.com%7C3344dd1ec8e44fc6d4a50
> 8dea85caa2a%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63913331
> 3118303655%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYi
> OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0
> %7C%7C%7C&sdata=GJTF8DNw%2FzYNsPz0lC%2FDFZpcCwqjuCmfVu7smim81x
> E%3D&reserved=0
> ---
> net: mana: Create separate EQs for each vPort
>
> This commit modifies the mana driver to prepare for assigning vPorts to dedicated
> MSI-X vectors by removing EQ sharing among vPorts. It moves the EQ definition
> from the mana_context to the mana_port_context and exports EQ lifecycle
> functions for use by the MANA RDMA driver.
>
> > diff --git a/drivers/infiniband/hw/mana/qp.c
> > b/drivers/infiniband/hw/mana/qp.c index 645581359cee..6f1043383e8c
> > 100644
> > --- a/drivers/infiniband/hw/mana/qp.c
> > +++ b/drivers/infiniband/hw/mana/qp.c
> > @@ -168,7 +168,15 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp,
> struct ib_pd *pd,
> > cq_spec.gdma_region = cq->queue.gdma_region;
> > cq_spec.queue_size = cq->cqe * COMP_ENTRY_SIZE;
> > cq_spec.modr_ctx_id = 0;
> > - eq = &mpc->ac->eqs[cq->comp_vector];
> > + /* EQs are created when a raw QP configures the vport.
> > + * A raw QP must be created before creating rwq_ind_tbl.
> > + */
> > + if (!mpc->eqs) {
> > + ret = -EINVAL;
> > + i--;
> > + goto fail;
> > + }
> > + eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];
>
> Is it possible for the EQs to be destroyed while this RSS QP is still actively using
> them?
No. The EQ id is consumed only at creation time - it is passed to firmware as cq_spec.attached_eq during mana_create_wq_obj(). After that call the CQ-to-EQ binding lives entirely in firmware.
The kernel never dereferences mpc->eqs again for that RSS QP's lifetime, so there is no ongoing kernel-side access to the EQ struct from an active RSS QP.
>
> If the EQs are created by the Ethernet interface being brought up, or by a RAW
> QP configuring the vport, this RSS QP will attach to them without incrementing
> pd->vport_use_count or taking any vport reference count.
This is by design. The RSS QP (RX side) does not take a vport reference, and symmetrically mana_ib_destroy_qp_rss() does not release one either. Only raw QPs (SQ side) take and release vport
references. The refcount is balanced: RSS QPs are pure consumers of an already-configured vport.
>
> If the Ethernet interface is subsequently brought down, or the RAW QP is
> destroyed, mana_destroy_eq() will be called, freeing the mpc->eqs array and
> destroying the underlying DMA regions while this RSS QP remains active. This
> regression could allow the hardware to DMA completion events into freed EQ
> memory.
Destroying the raw QP also calls mana_uncfg_vport(), which deconfigures the vport entirely. After that, firmware will not route any traffic or generate completions on this vport, so there are no
in-flight DMA writes to the EQ. This is the same pre-existing behavior: the raw QP has always been the vport lifecycle anchor, and destroying it while an RSS QP is active would have broken the
vport regardless - this patch does not change that relationship. Before this patch the EQs simply outlived the vport (device lifetime vs vport lifetime), which masked the dependency but did not
make the out-of-order teardown any safer at the vport level.
>
> Additionally, since mpc->eqs is accessed here without holding pd->vport_mutex,
> could a concurrent teardown of the EQs lead to a use-after-free when reading
> eq->eq->id?
The RDMA core serializes QP creation and destruction on the same device context. A concurrent teardown would require destroying the raw QP on the same PD simultaneously with RSS QP creation,
which the IB core does not permit. The !mpc->eqs check is a defensive guard against wrong call ordering (creating an RSS QP before any raw QP), not a synchronization point for concurrent access.
^ permalink raw reply
* Re: [PATCH v2 bpf 1/6] bpf: tcp: Fix type confusion in bpf_tcp_sock().
From: Kuniyuki Iwashima @ 2026-05-04 21:53 UTC (permalink / raw)
To: bot+bpf-ci
Cc: martin.lau, daniel, ast, andrii, eddyz87, memxor, john.fastabend,
sdf, song, yonghong.song, jolsa, edumazet, kuni1840, bpf, netdev,
melotti, martin.lau, clm, ihor.solodrai
In-Reply-To: <f3fb214ed73246731c525dfdccdcd379bd6d4f1e4570eba0cd19d56f888f9b0a@mail.kernel.org>
On Mon, May 4, 2026 at 2:50 PM <bot+bpf-ci@kernel.org> wrote:
>
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index bc96c18df4e0..cd88633f8dc1 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -7475,7 +7475,7 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
> >
> > BPF_CALL_1(bpf_tcp_sock, struct sock *, sk)
> > {
> > - if (sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP)
> > + if (sk_fullsock(sk) && sk_is_tcp(sk))
> > return (unsigned long)sk;
> >
> > return (unsigned long)NULL;
>
> While this patch fixes bpf_tcp_sock(), does sol_tcp_sockopt() remain
It seems Sashiko does not have context on the following patches.
All helper functions mentioned in the review comment are fixed
in the later patches separately.
> vulnerable? Since ctx->sk evaluates to PTR_TO_SOCKET in a CGROUP_SOCKOPT
> program and is directly accepted by bpf_setsockopt() and bpf_getsockopt()
> via ARG_PTR_TO_BTF_ID_SOCK_COMMON, can a user create a RAW socket with
> IPPROTO_TCP and directly pass ctx->sk to bpf_setsockopt(), bypassing
> bpf_tcp_sock() entirely?
>
> Looking at sol_tcp_sockopt():
>
> static int sol_tcp_sockopt(struct sock *sk, int optname,
> char *optval, int *optlen,
> bool getopt)
> {
> if (sk->sk_protocol != IPPROTO_TCP)
> return -EINVAL;
>
> This appears to only check sk->sk_protocol == IPPROTO_TCP without verifying
> sk->sk_type == SOCK_STREAM, which would allow the same slab out-of-bounds
> access through a different code path.
>
> Additionally, do bpf_skc_to_tcp_sock() and bpf_skc_to_tcp6_sock() suffer
> from the same type confusion? They appear to check sk->sk_protocol ==
> IPPROTO_TCP without verifying sk->sk_type == SOCK_STREAM.
>
> Could a BPF program pass a RAW socket with IPPROTO_TCP to these helpers
> and receive a valid BTF pointer to struct tcp_sock, allowing out-of-bounds
> slab reads?
>
> Looking at bpf_skc_to_tcp_sock():
>
> BPF_CALL_1(bpf_skc_to_tcp_sock, struct sock *, sk)
> {
> if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP)
> return (unsigned long)sk;
>
> return (unsigned long)NULL;
> }
>
> And bpf_skc_to_tcp6_sock():
>
> BPF_CALL_1(bpf_skc_to_tcp6_sock, struct sock *, sk)
> {
> /* tcp6_sock type is not generated in dwarf and hence btf,
> * trigger an explicit type generation here.
> */
> BTF_TYPE_EMIT(struct tcp6_sock);
> if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP &&
> sk->sk_family == AF_INET6)
> return (unsigned long)sk;
>
> Finally, does bpf_mptcp_sock_from_subflow() in net/mptcp/bpf.c also need
> a similar update? It checks sk->sk_protocol == IPPROTO_TCP and immediately
> evaluates sk_is_mptcp(sk).
>
> Since sk_is_mptcp() indiscriminately casts the socket to struct tcp_sock
> and accesses tcp_sk(sk)->is_mptcp, passing a RAW socket could trigger an
> immediate out-of-bounds slab read. If this memory happens to be non-zero,
> it might proceed to call mptcp_subflow_ctx(sk)->conn, which would read a
> garbage pointer from out-of-bounds memory:
>
> struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
> {
> if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
> return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
>
> return NULL;
> }
>
> These issues were raised by sashiko-bot@kernel.org in v1:
> https://lore.kernel.org/bpf/20260501184452.7F46FC2BCB4@smtp.kernel.org/
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25344293379
^ permalink raw reply
* Re: [PATCH iwl-net] idpf: fix RSS LUT memcpy size
From: Jacob Keller @ 2026-05-04 22:03 UTC (permalink / raw)
To: Larysa Zaremba
Cc: intel-wired-lan, Przemek Kitszel, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Joshua Hay,
Willem de Bruijn, Alice Michael, netdev, linux-kernel,
Aleksandr Loktionov, Tony Nguyen
In-Reply-To: <afiem5uwBus61bLb@soc-5CG4396X81.clients.intel.com>
On 5/4/2026 6:26 AM, Larysa Zaremba wrote:
> On Thu, Apr 30, 2026 at 09:38:44AM -0700, Jacob Keller wrote:
>> On 4/29/2026 12:42 AM, Larysa Zaremba wrote:
>>> Based on the following feedback from Sashiko (received for iXD phase 1
>>> patchset, but valid for the net tree):
>>>
>>> "Is the bounds check xn_params.recv_mem.iov_len < lut_buf_size sufficient?
>>> Since lut_buf_size only represents the size of the array elements, should
>>> this check instead verify that the payload is at least
>>> sizeof(struct virtchnl2_rss_lut) + lut_buf_size?
>>>
>>> [...]
>>>
>>> Does memcpy copy the correct amount of data here? rss_lut_size stores the
>>> number of 32-bit entries, not the size in bytes. Should it use
>>> lut_buf_size or rss_data->rss_lut_size * sizeof(u32) instead?"
>>>
>>> After inspecting the code, it was concluded that RSS memcpy size is in fact
>>> 4 times smaller than it has to be, since a single array entry in a u32, and
>>> rss_data->rss_lut_size is clearly used as an array size. Required Rx buffer
>>> size is also too small, but this is a common issue in the idpf code.
>>>
>>> Use a full buffer size (lut_buf_size) instead of the array length
>>> (rss_data->rss_lut_size) when doing memcpy of RSS lookup table.
>>> While at it, increase required Rx buffer size to a whole flex-array
>>> containing structure instead of just the array.
>>>
>>> Link: https://sashiko.dev/#/patchset/20260323174052.5355-1-larysa.zaremba%40intel.com?part=8
>>> Fixes: 95af467d9a4e ("idpf: configure resources for RX queues")
>>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>>> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
>>> ---
>>> drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
>>> index be66f9b2e101..a97d2e9b54d4 100644
>>> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
>>> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
>>> @@ -2916,7 +2916,7 @@ int idpf_send_get_set_rss_lut_msg(struct idpf_adapter *adapter,
>>> return -EIO;
>>>
>>> lut_buf_size = le16_to_cpu(recv_rl->lut_entries) * sizeof(u32);
>>> - if (reply_sz < lut_buf_size)
>>> + if (reply_sz < lut_buf_size + sizeof(struct virtchnl2_rss_lut))
>>
>> This feels like it should be using struct_size or flex_array_size...
>>
>
> struct_size() does not really fit here, as lut_buf_size is needed later for
> flex-array-only memcpy, but flex_array_size() I can use.
>
Right. I am mostly thinking in terms of the intent of the safety
mechanisms provided by these macros. (part of what they do is prevent
accidental overflow by capping at SIZE_T_MAX for example).
>>> return -EIO;
>>>
>>> /* size didn't change, we can reuse existing lut buf */
>>> @@ -2933,7 +2933,7 @@ int idpf_send_get_set_rss_lut_msg(struct idpf_adapter *adapter,
>>> }
>>>
>>> do_memcpy:
>>> - memcpy(rss_data->rss_lut, recv_rl->lut, rss_data->rss_lut_size);
>>> + memcpy(rss_data->rss_lut, recv_rl->lut, lut_buf_size);
>>>
>>> return 0;
>>> }
>>
^ permalink raw reply
* [PATCH net v1] net/mlx5: Fix HWS action unwind NULL dereference
From: Prathamesh Deshpande @ 2026-05-04 22:06 UTC (permalink / raw)
To: Saeed Mahameed, Leon Romanovsky
Cc: Moshe Shemesh, Tariq Toukan, Yevgeny Kliteynik, Jakub Kicinski,
netdev, linux-rdma, linux-kernel, Prathamesh Deshpande
mlx5_fs_fte_get_hws_actions() stores some destination actions in
fs_actions[] before checking whether action creation succeeded.
If creating a table-number or range destination action fails, or if
fetching a sampler destination action fails, dest_action is NULL but
num_fs_actions has already been incremented. The shared error path then
calls mlx5_fs_destroy_fs_action(), which dereferences fs_action->action
to get the HWS action type, causing a NULL pointer dereference while
unwinding the original failure.
Track whether the current destination action needs fs_actions[] cleanup,
but append it only after dest_action has been validated.
Fixes: 2ec6786ad0a6b ("net/mlx5: fs, add HWS fte API functions")
Fixes: 32e658c84b6d ("net/mlx5: fs, add support for dest flow sampler HWS action")
Signed-off-by: Prathamesh Deshpande <prathameshdeshpande7@gmail.com>
---
.../mellanox/mlx5/core/steering/hws/fs_hws.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
index aca77853abb8..fb12513dd83d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
@@ -950,6 +950,7 @@ static int mlx5_fs_fte_get_hws_actions(struct mlx5_flow_root_namespace *ns,
struct mlx5_flow_destination *attr = &dst->dest_attr;
bool type_uplink =
attr->type == MLX5_FLOW_DESTINATION_TYPE_UPLINK;
+ bool record_fs_action = false;
if (num_fs_actions == MLX5_FLOW_CONTEXT_ACTION_MAX ||
num_dest_actions == MLX5_FLOW_CONTEXT_ACTION_MAX) {
@@ -973,11 +974,11 @@ static int mlx5_fs_fte_get_hws_actions(struct mlx5_flow_root_namespace *ns,
break;
dest_action = mlx5_fs_create_dest_action_table_num(fs_ctx,
dst);
- fs_actions[num_fs_actions++].action = dest_action;
+ record_fs_action = true;
break;
case MLX5_FLOW_DESTINATION_TYPE_RANGE:
dest_action = mlx5_fs_create_dest_action_range(ctx, dst);
- fs_actions[num_fs_actions++].action = dest_action;
+ record_fs_action = true;
break;
case MLX5_FLOW_DESTINATION_TYPE_UPLINK:
case MLX5_FLOW_DESTINATION_TYPE_VPORT:
@@ -988,9 +989,7 @@ static int mlx5_fs_fte_get_hws_actions(struct mlx5_flow_root_namespace *ns,
dest_action =
mlx5_fs_get_dest_action_sampler(fs_ctx,
dst);
- fs_actions[num_fs_actions].action = dest_action;
- fs_actions[num_fs_actions++].sampler_id =
- dst->dest_attr.sampler_id;
+ record_fs_action = true;
break;
default:
err = -EOPNOTSUPP;
@@ -1000,6 +999,13 @@ static int mlx5_fs_fte_get_hws_actions(struct mlx5_flow_root_namespace *ns,
err = -ENOMEM;
goto free_actions;
}
+ if (record_fs_action) {
+ fs_actions[num_fs_actions].action = dest_action;
+ if (attr->type == MLX5_FLOW_DESTINATION_TYPE_FLOW_SAMPLER)
+ fs_actions[num_fs_actions].sampler_id =
+ dst->dest_attr.sampler_id;
+ num_fs_actions++;
+ }
dest_actions[num_dest_actions++].dest = dest_action;
}
}
--
2.43.0
^ permalink raw reply related
* RE: [EXTERNAL] Re: [PATCH net-next v6 1/6] net: mana: Create separate EQs for each vPort
From: Long Li @ 2026-05-04 22:07 UTC (permalink / raw)
To: Simon Horman
Cc: Konstantin Taranov, kuba@kernel.org, davem@davemloft.net,
pabeni@redhat.com, edumazet@google.com, andrew+netdev@lunn.ch,
jgg@ziepe.ca, leon@kernel.org, Haiyang Zhang, KY Srinivasan,
wei.liu@kernel.org, Dexuan Cui, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20260502152354.289044-2-horms@kernel.org>
> From: 'Simon Horman' <horms@kernel.org>
>
> This is an AI-generated review of your patch. The human sending this email has
> considered the AI review valid, or at least plausible.
> Full review at:
> https://sashiko.d/
> ev%2F&data=05%7C02%7Clongli%40microsoft.com%7C36b10107f9774abb6894
> 08dea85f3721%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6391333
> 24077887564%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYi
> OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0
> %7C%7C%7C&sdata=cS3FmP7jK6ftjo74XWvtBHNXYa7YKlthqLgzEudcmb4%3D&re
> served=0
> ---
> net: mana: Create separate EQs for each vPort
>
> This change moves EQ ownership from struct mana_context to struct
> mana_port_context so each vPort can have dedicated EQs, and exports
> mana_create_eq()/mana_destroy_eq() for use by the MANA RDMA driver.
>
> > diff --git a/drivers/infiniband/hw/mana/main.c
> > b/drivers/infiniband/hw/mana/main.c
> > index ac5e75dd3494..60cc02e4ad10 100644
> > --- a/drivers/infiniband/hw/mana/main.c
> > +++ b/drivers/infiniband/hw/mana/main.c
> > @@ -20,8 +20,10 @@ void mana_ib_uncfg_vport(struct mana_ib_dev *dev,
> struct mana_ib_pd *pd,
> > pd->vport_use_count--;
> > WARN_ON(pd->vport_use_count < 0);
> >
> > - if (!pd->vport_use_count)
> > + if (!pd->vport_use_count) {
> > + mana_destroy_eq(mpc);
> > mana_uncfg_vport(mpc);
> > + }
> >
> > mutex_unlock(&pd->vport_mutex);
> > }
> > @@ -55,15 +57,21 @@ int mana_ib_cfg_vport(struct mana_ib_dev *dev, u32
> port, struct mana_ib_pd *pd,
> > return err;
> > }
> >
> > - mutex_unlock(&pd->vport_mutex);
> >
> > pd->tx_shortform_allowed = mpc->tx_shortform_allowed;
> > pd->tx_vp_offset = mpc->tx_vp_offset;
> > + err = mana_create_eq(mpc);
> > + if (err) {
> > + mana_uncfg_vport(mpc);
> > + pd->vport_use_count--;
> > + }
> > +
> > + mutex_unlock(&pd->vport_mutex);
> >
> > ibdev_dbg(&dev->ib_dev, "vport handle %llx pdid %x doorbell_id %x\n",
> > mpc->port_handle, pd->pdn, doorbell_id);
>
> On the failure path, mana_uncfg_vport() is called and pd->vport_use_count is
> decremented, but execution falls through to the ibdev_dbg() line that prints the
> vport handle as if it were a success. Should this debug print be skipped on the
> error path so logs are not ambiguous?
This is fair point. The ibdev_dbg could be guarded with if (!err) for clarity. That said, ibdev_dbg is compiled out in non-debug builds and the error itself is returned to the caller, so this is
cosmetic. Happy to add the guard if you feel strongly.
>
> Also, pd->tx_shortform_allowed and pd->tx_vp_offset are assigned from mpc
> before the mana_create_eq() attempt and are not cleared when that call fails.
> Callers are expected to gate on pd->vport_use_count, but is it worth resetting
> these fields on failure so the state matches the pre-patch behavior?
These fields are only consumed when pd->vport_use_count > 0. On failure the count is decremented back to 0, so the stale values are never read. The next successful mana_ib_cfg_vport() call will
overwrite them. This matches pre-patch behavior - pre-patch also assigned these fields unconditionally and relied on the use count to gate access.
>
> The mutex_unlock(&pd->vport_mutex) has moved from before the field
> assignments to after mana_create_eq(). That now holds pd->vport_mutex across
> GFP_KERNEL allocations, debugfs directory creation, and multiple
> mana_gd_create_mana_eq() firmware RPCs, and establishes the lock order
> pd->vport_mutex -> apc->vport_mutex via mana_uncfg_vport() on the error
> path. Is the extended critical section intentional?
Yes, this is intentional. Pre-patch, pd->vport_mutex was released before assigning pd->tx_shortform_allowed and pd->tx_vp_offset, which was a data race - a concurrent raw
QP creation on the same PD could read partially-initialized state. The extended critical section ensures the vport is fully configured (including EQs) before any concurrent user can observe
vport_use_count > 0.
>
> >
> > - return 0;
> > + return err;
> > }
> >
> > int mana_ib_alloc_pd(struct ib_pd *ibpd, struct ib_udata *udata) diff
> > --git a/drivers/infiniband/hw/mana/qp.c
> > b/drivers/infiniband/hw/mana/qp.c index 645581359cee..6f1043383e8c
> > 100644
> > --- a/drivers/infiniband/hw/mana/qp.c
> > +++ b/drivers/infiniband/hw/mana/qp.c
> > @@ -168,7 +168,15 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp,
> struct ib_pd *pd,
> > cq_spec.gdma_region = cq->queue.gdma_region;
> > cq_spec.queue_size = cq->cqe * COMP_ENTRY_SIZE;
> > cq_spec.modr_ctx_id = 0;
> > - eq = &mpc->ac->eqs[cq->comp_vector];
> > + /* EQs are created when a raw QP configures the vport.
> > + * A raw QP must be created before creating rwq_ind_tbl.
> > + */
> > + if (!mpc->eqs) {
> > + ret = -EINVAL;
> > + i--;
> > + goto fail;
> > + }
> > + eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];
>
> Can the mpc->eqs read race with a concurrent free? mana_ib_create_qp_rss()
> runs without pd->vport_mutex or RTNL, but mpc->eqs is freed by
> mana_destroy_eq() from two paths:
>
> mana_ib_uncfg_vport() (under pd->vport_mutex, on last raw-QP destroy)
> mana_dealloc_queues() (under RTNL, on netdev down)
>
> both of which do:
>
> kfree(apc->eqs);
> apc->eqs = NULL;
>
> with no RCU grace period or reader-visible synchronization. If CPU-A passes
> the !mpc->eqs check after CPU-B begins ip link set dev X down, does CPU-A then
> dereference freed memory via mpc->eqs[...].eq->id?
These two paths cannot run concurrently with RDMA QP creation on the same port. mana_cfg_vport() enforces mutual exclusion between Ethernet and RDMA via apc->vport_use_count:
mutex_lock(&apc->vport_mutex);
if (apc->vport_use_count > 0) {
mutex_unlock(&apc->vport_mutex);
return -EBUSY;
}
apc->vport_use_count++;
If RDMA holds the vport (created a raw QP), Ethernet cannot bring the interface up, so mana_dealloc_queues() cannot run. If Ethernet holds the vport, mana_ib_cfg_vport() → mana_cfg_vport()
returns -EBUSY and no RDMA raw QP is created, so mpc->eqs belongs exclusively to Ethernet.
The mana_ib_uncfg_vport() path requires destroying the last raw QP on the PD, which means no new RDMA QPs should be in flight on that PD. The IB core serializes QP creation/destruction on the
same device context.
>
> Separately, what populates mpc->eqs for an RDMA-only RSS QP user on a probed-
> but-idle Ethernet port? Pre-patch mana_probe() called
> mana_create_eq(ac) unconditionally, so ac->eqs existed for the device lifetime.
> After this patch the only creators are mana_alloc_queues() (Ethernet up) and
> mana_ib_cfg_vport() (raw QP). An RDMA application that uses only RSS QPs and
> never creates a raw QP will now get -EINVAL here where it used to succeed. Is
> this intended, and should the commit log mention it?
This is intentional. An RSS-only RDMA application (no raw QP) could never have worked in practice: mana_ib_create_qp_rss() calls mana_create_wq_obj(mpc, mpc->port_handle, ...) which requires the
vport to be configured. Without a raw QP calling mana_ib_cfg_vport() → mana_cfg_vport(), mpc->port_handle is INVALID_MANA_HANDLE and the firmware call would fail. The -EINVAL is a cleaner early
error for a path that was already broken.
>
> The adjacent comment:
>
> /* EQs are created when a raw QP configures the vport.
> * A raw QP must be created before creating rwq_ind_tbl.
> */
>
> omits the Ethernet-up path that also populates mpc->eqs. Would it be clearer to
> describe both creators?
The comment is in the RDMA code path. Due to the mana_cfg_vport() mutual exclusion described above, when RDMA is executing this code, it owns the vport - so the EQs were created by the raw QP,
not by Ethernet. The Ethernet path is not reachable when RDMA holds the port. The comment is accurate for the context it appears in.
>
> There is also a behavior change in the index expression:
>
> eq = &mpc->eqs[cq->comp_vector % mpc->num_queues];
>
> Pre-patch this was ac->eqs[cq->comp_vector] sized by gc->max_num_queues.
> Now comp_vector is folded modulo mpc->num_queues, which is tunable via
> ethtool -L. Userspace that used distinct comp_vector values to hit distinct EQs
> will silently alias when comp_vector >= num_queues. Should this be documented
> or rejected with -EINVAL rather than silently wrapped?
The modulo wrap is the correct behavior. The EQ array is now sized to mpc->num_queues (the actual configured queue count), not the hardware maximum. Rejecting with -EINVAL would break
applications that set comp_vector based on the device's total num_comp_vectors - which is the standard RDMA practice. Wrapping is what other drivers do in this situation (e.g., mlx5). Pre-patch
the wrap just happened to be unnecessary because the array was oversized to the hardware max.
>
> Can mpc->num_queues be 0 at this point? mana_set_channels() does not reject
> new_count==0, and kzalloc_objs(struct mana_eq, 0) returns ZERO_SIZE_PTR,
> which passes the !mpc->eqs check. During the window between
> mana_create_eq() and the subsequent netif_set_real_num_tx_queues() failing, a
> concurrent RDMA QP create would compute
> cq->comp_vector % 0 here. Should mpc->num_queues be validated alongside
> mpc->eqs?
num_queues cannot be 0. The ethtool core validates this before calling the driver - ethnl_set_channels() rejects the request if combined_count is 0 and there are no separate rx/tx channels
(which MANA doesn't use). Additionally, num_queues is initialized at probe time to min(gc->max_num_queues_vport, MANA_DEF_NUM_QUEUES) where MANA_DEF_NUM_QUEUES is 16 and the hardware always
reports at least 1 queue.
>
> The placement of the !mpc->eqs check is inside the per-iteration loop over
> ind_tbl_size, but mpc->eqs cannot change across iterations, so the check is only
> meaningful at i==0. It works today because i-- then makes i = -1 and the cleanup
> while (i-- > 0) skips. Would hoisting the check above the loop be clearer and less
> fragile against refactoring?
>
> > cq_spec.attached_eq = eq->eq->id;
> >
> > ret = mana_create_wq_obj(mpc, mpc->port_handle, GDMA_RQ,
> @@ -317,7
> > +325,11 @@ static int mana_ib_create_qp_raw(struct ib_qp *ibqp, struct ib_pd
> *ibpd,
> > cq_spec.queue_size = send_cq->cqe * COMP_ENTRY_SIZE;
> > cq_spec.modr_ctx_id = 0;
> > eq_vec = send_cq->comp_vector;
> > - eq = &mpc->ac->eqs[eq_vec];
> > + if (!mpc->eqs) {
> > + err = -EINVAL;
> > + goto err_destroy_queue;
> > + }
> > + eq = &mpc->eqs[eq_vec % mpc->num_queues];
>
> The same mpc->eqs TOCTOU concern applies here: the check and subsequent
> dereference occur without pd->vport_mutex or RTNL held, so a concurrent
> mana_dealloc_queues() on the Ethernet side can free mpc->eqs between the
> check and the index access. Is there synchronization that prevents this?
Same answer as above: mana_cfg_vport() mutual exclusion prevents this. In mana_ib_create_qp_raw(), mana_ib_cfg_vport() is called just before this code, which calls mana_cfg_vport(). If Ethernet
holds the port, that call returns -EBUSY and we never reach the mpc->eqs access. If we reach it, RDMA owns the vport and Ethernet cannot tear it down.
>
> The same comp_vector % num_queues aliasing and num_queues==0 divide
> concerns apply here as well.
>
> > cq_spec.attached_eq = eq->eq->id;
> >
> > err = mana_create_wq_obj(mpc, mpc->port_handle, GDMA_SQ,
> &wq_spec,
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index a654b3699c4c..6c709f8b875d 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
>
> [ ... ]
>
> > -static int mana_create_eq(struct mana_context *ac)
> > +int mana_create_eq(struct mana_port_context *apc)
> > {
> > - struct gdma_dev *gd = ac->gdma_dev;
> > + struct gdma_dev *gd = apc->ac->gdma_dev;
> > struct gdma_context *gc = gd->gdma_context;
> > struct gdma_queue_spec spec = {};
> > int err;
> > int i;
> >
> > - ac->eqs = kzalloc_objs(struct mana_eq, gc->max_num_queues);
> > - if (!ac->eqs)
> > + WARN_ON(apc->eqs);
> > + apc->eqs = kzalloc_objs(struct mana_eq, apc->num_queues);
> > + if (!apc->eqs)
> > return -ENOMEM;
>
> WARN_ON(apc->eqs) is used as a defensive assertion but execution proceeds to
> overwrite apc->eqs unconditionally. If the invariant is violated, the previous array
> and all HW EQs it referenced are leaked.
> Should this instead bail out with an error or call mana_destroy_eq() first to keep
> the function idempotent against future callers?
The WARN_ON is a development assertion to catch double-init bugs. If it fires, there is already a logic error in the caller. Silently cleaning up and proceeding would mask the root cause. The
standard kernel pattern for "this must never happen" is WARN_ON + continue, not silent recovery. If we wanted to be stricter, returning -EBUSY would be reasonable, but the current approach is
consistent with kernel convention for invariant checks.
>
> [ ... ]
>
> > @@ -3326,6 +3349,9 @@ static int mana_dealloc_queues(struct net_device
> *ndev)
> > mana_fence_rqs(apc);
> >
> > /* Even in err case, still need to cleanup the vPort */
> > + mana_destroy_rxqs(apc);
> > + mana_destroy_txq(apc);
> > + mana_destroy_eq(apc);
> > mana_destroy_vport(apc);
>
> What is the intended interaction with outstanding RDMA QPs here? An RDMA
> user that created an RSS QP while the Ethernet netdev was up sees
> mpc->eqs populated by mana_alloc_queues(). When the admin issues
> ip link set dev N down, mana_dealloc_queues() -> mana_destroy_eq() destroys
> those EQs and frees mpc->eqs while the RDMA QPs are still alive, leaving the QPs
> with dangling attached_eq IDs at the hardware level and stale kernel references.
This scenario cannot occur. As described above, mana_cfg_vport() enforces mutual exclusion - Ethernet and RDMA cannot hold the same port simultaneously. If Ethernet is up, RDMA raw QP creation
on that port returns -EBUSY. If RDMA holds the port, mana_alloc_queues() → mana_create_vport() → mana_cfg_vport() returns -EBUSY and the interface fails to come up. There is no state where both
have created EQs on the same mpc.
>
> Pre-patch ac->eqs lived for the full mana_context lifetime and was torn down
> only in mana_remove(). Is unconditionally destroying the EQs on netdev-down
> the intended new behavior, and if so how are concurrent RDMA consumers
> expected to recover?
Yes, destroying EQs on netdev-down is the intended new behavior, and no RDMA recovery path is needed because the scenario has no concurrent RDMA consumers.
mana_cfg_vport() enforces mutual exclusion at the hardware port level via apc->vport_use_count - it returns -EBUSY if the port is already held. Ethernet and RDMA cannot hold the same port
simultaneously:
- If Ethernet is up → RDMA raw QP creation fails at mana_cfg_vport() with -EBUSY → no RDMA EQs or QPs exist on that port → netdev-down destroys only Ethernet's own EQs.
- If RDMA holds the port → mana_alloc_queues() → mana_create_vport() → mana_cfg_vport() returns -EBUSY → interface never comes up → mana_dealloc_queues() never runs.
Pre-patch, the device-lifetime EQs in ac->eqs were shared across all ports and both subsystems, which masked this exclusivity - the EQs were always present regardless of who owned the port.
Post-patch, each port owns its EQs, and they follow the lifecycle of whoever holds the port. The exclusion guarantee means there is nothing to recover from.
^ permalink raw reply
* RE: [EXTERNAL] Re: [PATCH net-next v6 1/6] net: mana: Create separate EQs for each vPort
From: Long Li @ 2026-05-04 22:08 UTC (permalink / raw)
To: Simon Horman
Cc: Konstantin Taranov, kuba@kernel.org, davem@davemloft.net,
pabeni@redhat.com, edumazet@google.com, andrew+netdev@lunn.ch,
jgg@ziepe.ca, leon@kernel.org, Haiyang Zhang, KY Srinivasan,
wei.liu@kernel.org, Dexuan Cui, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20260502152929.GL15617@horms.kernel.org>
> On Sat, May 02, 2026 at 04:23:55PM +0100, Simon Horman wrote:
> > From: 'Simon Horman' <horms@kernel.org>
> >
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at:
> > https://sash/
> >
> iko.dev%2F&data=05%7C02%7Clongli%40microsoft.com%7C50f9138d30ca49fb0
> 5b
> >
> 708dea85f9e35%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C639133
> 32578
> >
> 9863881%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIw
> LjAuMD
> >
> AwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7
> C&sd
> > ata=1Ew7dYw%2B7zQjROcj63hnOYFUfak20Pi3ytzOf2J0JWg%3D&reserved=0
>
> Sorry about this, there was supposed to be some different text here.
I have replied to both comments.
Thank you,
Long
>
> This review is available at
> https://netdev-/
> ai.bots.linux.dev%2Fsashiko%2F&data=05%7C02%7Clongli%40microsoft.com%7
> C50f9138d30ca49fb05b708dea85f9e35%7C72f988bf86f141af91ab2d7cd011db47
> %7C1%7C0%7C639133325789877695%7CUnknown%7CTWFpbGZsb3d8eyJFbXB
> 0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIs
> IldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=F4%2FR6eEmKkHI0%2FkuGfvuaM
> 42oss8KCUb9J5Bw6B682Y%3D&reserved=0
> And I apologise that it overlaps with the review from
> https://sashiko.d/
> ev%2F&data=05%7C02%7Clongli%40microsoft.com%7C50f9138d30ca49fb05b70
> 8dea85f9e35%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63913332
> 5789886888%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYi
> OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0
> %7C%7C%7C&sdata=g%2BFgPtQXNQSqC%2ByHDua2twXj%2BufRZ8yCze757NpY
> vU8%3D&reserved=0
> which I also posted.
^ permalink raw reply
* Re: [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS
From: Michael Nemanov @ 2026-05-04 22:09 UTC (permalink / raw)
To: Chuck Lever, Trond Myklebust, Anna Schumaker, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman
Cc: linux-nfs, netdev, Chuck Lever
In-Reply-To: <20260504-sunrpc-tls-clnt-pin-v1-0-197f359c6072@oracle.com>
On 04/05/2026 13:28, Chuck Lever wrote:
> Patch 2 fixes a use-after-free Michael Nemanov hit on an mTLS mount
> whose client certificate the server rejected.
Reviewed and tested both patches.
Confirmed 3-sec delayed work was happening multiple times without UAF.
Thank you.
Tested-by: Michael Nemanov <michael.nemanov@vastdata.com>
Reviewed-by: Michael Nemanov <michael.nemanov@vastdata.com>
^ permalink raw reply
* Re: [PATCH] ixgbe: E610: do not fill EEE lp_advertised from local PHY caps
From: Jacob Keller @ 2026-05-04 22:12 UTC (permalink / raw)
To: David CARLIER, Andrew Lunn, Jagielski, Jedrzej
Cc: Tony Nguyen, Przemek Kitszel, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jedrzej Jagielski,
Aleksandr Loktionov, intel-wired-lan, netdev, linux-kernel
In-Reply-To: <CA+XhMqxjWOXVJXGUx5BE+U0S0SOOoSwaKu8eWigS=J5EfFejcQ@mail.gmail.com>
On 5/4/2026 7:05 AM, David CARLIER wrote:
> Hi Andrew,
>
> No E610 here, found it by reading the code - the X550 path
> (ixgbe_get_eee_fw) uses a separate FW_PHY_ACT_UD_2 activity and
> ixgbe_lp_map[] for partner data, the E610 path just feeds
> pcaps.eee_cap from REPORT_ACTIVE_CFG into lp_advertised. None of
> the IXGBE_ACI_REPORT_* modes return partner info so that field
> can't be right.
>
> The set path goes hw->mac.ops.setup_eee() ->
> ixgbe_aci_set_phy_cfg(),
> so negotiation is in the firmware. eee_active / eee_enabled come
> from link.eee_status from the same FW, if those bits are right then
> negotiation works. Can't say more without hardware, Jedrzej or
> Aleksandr would know.
>
> Cheers
Hi David,
Thanks for the report and possible patch. The EEE support just merged,
and I believe the series has undergone testing. It is possible E610 is
significantly different from X550.
@Jedrzej,
Could you please look at this patch and the report from David and
confirm if we need this (or a different?) fix or if the code is correct
for E610 and explain why in that case?
Thanks,
Jake
^ permalink raw reply
* [PATCH net v1] net/mlx5: Fix HWS L2-to-L3 tunnel reformat release
From: Prathamesh Deshpande @ 2026-05-04 22:19 UTC (permalink / raw)
To: Saeed Mahameed, Leon Romanovsky
Cc: Moshe Shemesh, Mark Bloch, Tariq Toukan, Yevgeny Kliteynik,
Jakub Kicinski, netdev, linux-rdma, linux-kernel,
Prathamesh Deshpande
mlx5_cmd_hws_packet_reformat_alloc() allocates
MLX5_REFORMAT_TYPE_L2_TO_L3_TUNNEL objects from el2tol3tnl_pools with
MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L3.
The deallocation path uses el2tol2tnl_pools with
MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L2 instead. This releases the
packet-reformat entry through the wrong pool, corrupting pool accounting
and potentially moving the bulk entry onto the wrong pool list.
Use the matching L2-to-L3 tunnel pool and action type when releasing the
object.
Fixes: aecd9d1020e3 ("net/mlx5: fs, add HWS packet reformat API function")
Signed-off-by: Prathamesh Deshpande <prathameshdeshpande7@gmail.com>
---
drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
index aca77853abb8..60fbb048db10 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
@@ -1396,8 +1396,8 @@ static void mlx5_cmd_hws_packet_reformat_dealloc(struct mlx5_flow_root_namespace
pr_data->data_size);
break;
case MLX5_REFORMAT_TYPE_L2_TO_L3_TUNNEL:
- pr_pool = mlx5_fs_get_pr_encap_pool(dev, &hws_pool->el2tol2tnl_pools,
- MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L2,
+ pr_pool = mlx5_fs_get_pr_encap_pool(dev, &hws_pool->el2tol3tnl_pools,
+ MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L3,
pr_data->data_size);
break;
case MLX5_REFORMAT_TYPE_L3_TUNNEL_TO_L2:
--
2.43.0
^ permalink raw reply related
* RE: [EXTERNAL] Re: [PATCH net-next v6 2/6] net: mana: Query device capabilities and configure MSI-X sharing for EQs
From: Long Li @ 2026-05-04 22:21 UTC (permalink / raw)
To: Simon Horman
Cc: Konstantin Taranov, kuba@kernel.org, davem@davemloft.net,
pabeni@redhat.com, edumazet@google.com, andrew+netdev@lunn.ch,
jgg@ziepe.ca, leon@kernel.org, Haiyang Zhang, KY Srinivasan,
wei.liu@kernel.org, Dexuan Cui, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20260502150835.281887-1-horms@kernel.org>
> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Saturday, May 2, 2026 8:09 AM
> To: Long Li <longli@microsoft.com>
> Cc: 'Simon Horman' <horms@kernel.org>; Konstantin Taranov
> <kotaranov@microsoft.com>; kuba@kernel.org; davem@davemloft.net;
> pabeni@redhat.com; edumazet@google.com; andrew+netdev@lunn.ch;
> jgg@ziepe.ca; leon@kernel.org; Haiyang Zhang <haiyangz@microsoft.com>; KY
> Srinivasan <kys@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <DECUI@microsoft.com>; netdev@vger.kernel.org; linux-rdma@vger.kernel.org;
> linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH net-next v6 2/6] net: mana: Query device
> capabilities and configure MSI-X sharing for EQs
>
> From: 'Simon Horman' <horms@kernel.org>
>
> This is an AI-generated review of your patch. The human sending this email has
> considered the AI review valid, or at least plausible.
> Full review at:
> https://sashiko.d/
> ev%2F&data=05%7C02%7Clongli%40microsoft.com%7C1b8423bb5d15490288be
> 08dea85cb944%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6391333
> 13365211100%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYi
> OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0
> %7C%7C%7C&sdata=H6rPcEM62LmbWsXdpSVcVaaec3Wx6KJnTnYS0fBk6vY%3D
> &reserved=0
> ---
> net: mana: Query device capabilities and configure MSI-X sharing for EQs
>
> When querying the device, this adjusts the max number of queues to allow
> dedicated MSI-X vectors for each vPort, clamping to no less than
> MANA_DEF_NUM_QUEUES. MSI-X sharing among vPorts is disabled by default
> and enabled only when there are not enough vectors. It also renames
> mana_query_device_cfg() to mana_gd_query_device_cfg().
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
>
> [ ... ]
>
> > + /*
> > + * Adjust gc->max_num_queues returned from the SOC to allow
> dedicated
> > + * MSIx for each vPort. Clamp to no less than
> MANA_DEF_NUM_QUEUES.
> > + */
> > + max_num_queues = (gc->num_msix_usable - 1) / num_ports;
> > + max_num_queues = roundup_pow_of_two(max(max_num_queues, 1U));
>
> Does rounding up here inflate max_num_queues and unnecessarily force MSI-X
> sharing?
>
> For example, if there are 63 usable MSIs and 2 ports, the division yields 31.
> Rounding up gives 32. The subsequent check below:
>
> > + /*
> > + * Use dedicated MSIx for EQs whenever possible, use MSIx sharing for
> > + * Ethernet EQs when (max_num_queues * num_ports >
> num_msix_usable - 1)
> > + */
> > + max_num_queues = min(gc->max_num_queues, max_num_queues);
> > + if (max_num_queues * num_ports > gc->num_msix_usable - 1)
> > + gc->msi_sharing = true;
>
> would then evaluate to true (32 * 2 > 63) and force the driver into MSI-X shared
> mode. This seems to contradict the intent to use dedicated MSI-X whenever
> possible.
>
> Would it be better to use rounddown_pow_of_two() instead to ensure the
> calculated queues fit within the available dedicated vectors?
The roundup is intentional. The primary goal is to maximize the number of queues per vPort for throughput - power-of-2 queue counts align with RSS indirection table sizes and give the best
distribution. Dedicated MSI-X is preferred when it comes for free, but not at the cost of halving the queue count.
In your example (63 MSIs, 2 ports), rounddown would give 16 queues per port - nearly half the 31 that the hardware could support. Roundup gives 32 and triggers sharing mode, but sharing mode is
designed to work correctly and the queues still get serviced. The driver trades per-EQ interrupt exclusivity for better parallelism, which is the right tradeoff for network throughput on these
devices.
The sharing check exists as a fallback for exactly this situation - it is not a failure mode but a deliberate operating point.
>
> [ ... ]
>
> > @@ -1968,20 +2008,30 @@ static int mana_gd_setup(struct pci_dev *pdev)
> > if (err)
> > goto destroy_hwc;
> >
> > - err = mana_gd_query_max_resources(pdev);
> > + err = mana_gd_detect_devices(pdev);
> > if (err)
> > goto destroy_hwc;
> >
> > - err = mana_gd_setup_remaining_irqs(pdev);
> > - if (err) {
> > - dev_err(gc->dev, "Failed to setup remaining IRQs: %d", err);
> > - goto destroy_hwc;
> > - }
> > -
> > - err = mana_gd_detect_devices(pdev);
> > + err = mana_gd_query_max_resources(pdev);
> > if (err)
> > goto destroy_hwc;
> >
> > + if (!gc->msi_sharing) {
> > + gc->msi_bitmap = bitmap_zalloc(gc->num_msix_usable,
> GFP_KERNEL);
> > + if (!gc->msi_bitmap) {
> > + err = -ENOMEM;
> > + goto destroy_hwc;
> > + }
> > + /* Set bit for HWC */
> > + set_bit(0, gc->msi_bitmap);
> > + } else {
> > + err = mana_gd_setup_remaining_irqs(pdev);
> > + if (err) {
> > + dev_err(gc->dev, "Failed to setup remaining IRQs: %d",
> err);
> > + goto destroy_hwc;
> > + }
> > + }
>
> If msi_sharing is disabled, we allocate the msi_bitmap but skip calling
> mana_gd_setup_remaining_irqs().
>
> Since mana_gd_setup_hwc_irqs() only allocates a single vector for the hardware
> channel when dynamic allocation is supported, does this leave the device without
> interrupts for its Ethernet queues?
>
> If so, it seems this could lead to queue creation failures when the driver attempts
> to map uninitialized vectors. I notice this is fixed in a later patch in the series
> ("net: mana: Allocate interrupt context for each EQ when creating vPort"), but
> does leaving it out here break bisectability?
You're right that with pci_msix_can_alloc_dyn() returning true on MANA, the non-sharing path is reachable between patches 2 and 5. In that window, mana_gd_register_irq() would fail with -EINVAL
because irq_contexts for indices 1+ haven't been populated yet.
However, the failure is contained: mana_create_eq() returns an error, mana_alloc_queues() propagates it, and the interface fails to come up cleanly - no WARN, no crash, no data corruption. The
driver remains in a consistent state and succeeds once the full series is applied.
This is a new capability being built up across the series. The dedicated MSI-X mode did not exist before, so there is no regression from the pre-patch baseline - the pre-patch code always went
through mana_gd_setup_remaining_irqs() and operated in what is now called sharing mode. Restructuring the series to make non-sharing mode functional at each intermediate commit would require
squashing the GIC infrastructure (patches 3-4) into this patch, producing a single large change that is significantly harder to review.
I'd prefer to keep the logical separation as-is. If you feel strongly about strict bisectability, I could add a fallback in this patch that forces msi_sharing = true when the GIC allocator is
not yet available, and have patch 5 remove it - but that adds throwaway code to an intermediate commit.
^ permalink raw reply
* RE: [EXTERNAL] Re: [PATCH net-next v6 2/6] net: mana: Query device capabilities and configure MSI-X sharing for EQs
From: Long Li @ 2026-05-04 22:30 UTC (permalink / raw)
To: Simon Horman
Cc: Konstantin Taranov, kuba@kernel.org, davem@davemloft.net,
pabeni@redhat.com, edumazet@google.com, andrew+netdev@lunn.ch,
jgg@ziepe.ca, leon@kernel.org, Haiyang Zhang, KY Srinivasan,
wei.liu@kernel.org, Dexuan Cui, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20260502152649.292433-5-horms@kernel.org>
> From: 'Simon Horman' <horms@kernel.org>
>
> This is an AI-generated review of your patch. The human sending this email has
> considered the AI review valid, or at least plausible.
> Full review at:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fnetdev-
> ai.bots.linux.dev%2Fsashiko%2F&data=05%7C02%7Clongli%40microsoft.com%7
> Ca5d48bdebd0240e5d91308dea85ff3a8%7C72f988bf86f141af91ab2d7cd011db4
> 7%7C1%7C0%7C639133327234427106%7CUnknown%7CTWFpbGZsb3d8eyJFbX
> B0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbC
> IsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=4doWStoTQ4w7CbmhN3I294b%
> 2B9c3NNjDGySSmISHGZk4%3D&reserved=0
>
> I apologise for any overlap with this and the review of this patchset available at
> sashiko.dev. They both seem relevant and I'm unsure how to cleanly pass them
> both on.
>
> Both Sashiko instances also flagged concerns in the remaining patches of this
> patchset. But I am not forwarding them as they seem to be lower priority, and I
> think I've already created enough noise here.
>
> ---
> net: mana: Query device capabilities and configure MSI-X sharing for EQs
>
> This patch queries device capabilities early in GDMA probe and chooses between
> dedicated MSI-X per vPort or a shared-MSI-X mode based on the available vector
> count, introducing gc->max_num_queues_vport,
> gc->msi_sharing and gc->msi_bitmap, and renaming mana_query_device_cfg()
> to mana_gd_query_device_cfg() so it can be called from the GDMA layer.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > index 098fbda0d128..b96859e0aec9 100644
> > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > @@ -149,6 +149,9 @@ static int mana_gd_query_max_resources(struct
> pci_dev *pdev)
> > struct gdma_context *gc = pci_get_drvdata(pdev);
> > struct gdma_query_max_resources_resp resp = {};
> > struct gdma_general_req req = {};
> > + unsigned int max_num_queues;
> > + u8 bm_hostmode;
> > + u16 num_ports;
> > int err;
> >
> > mana_gd_init_req_hdr(&req.hdr, GDMA_QUERY_MAX_RESOURCES,
> @@ -194,6
> > +197,40 @@ static int mana_gd_query_max_resources(struct pci_dev *pdev)
> > if (gc->max_num_queues > gc->num_msix_usable - 1)
> > gc->max_num_queues = gc->num_msix_usable - 1;
> >
> > + err = mana_gd_query_device_cfg(gc, MANA_MAJOR_VERSION,
> MANA_MINOR_VERSION,
> > + MANA_MICRO_VERSION, &num_ports,
> &bm_hostmode);
> > + if (err)
> > + return err;
>
> Is this the same MANA_QUERY_DEV_CONFIG that mana_probe() issues shortly
> after? mana_probe() still contains:
>
> err = mana_gd_query_device_cfg(gc, MANA_MAJOR_VERSION,
> MANA_MINOR_VERSION,
> MANA_MICRO_VERSION, &num_ports,
> &bm_hostmode);
>
> so the same HWC round-trip runs twice on every probe with identical arguments,
> both populating gc->adapter_mtu.
>
> The commit message frames this rename as consolidation ("used at GDMA device
> probe time"), but the mana_probe() call-site is not removed. Was one of the two
> call-sites meant to go away?
>
> The relocation of debugfs_create_u16("adapter-MTU", ...) from
> mana_gd_query_device_cfg() into mana_probe() only makes sense if the function
> is now invoked more than once, which hints at the same duplication.
The two calls are intentional and serve different layers. The call in mana_gd_query_max_resources() runs at the GDMA layer during device setup - it needs num_ports to compute per-vPort MSI-X
allocation before any MANA-level structures exist. The call in mana_probe() runs at the MANA layer and consumes the results differently: it populates ac->num_ports, ac->bm_hostmode, and on
resume validates that the port count hasn't changed.
The HWC round-trip is lightweight (a single command/response exchange), and the function is renamed to mana_gd_ precisely because it now has callers at both layers. The debugfs move is the same
reason - the debugfs node should only be created once, so it moves to mana_probe() which runs once, rather than staying in the function that is now called from both sites.
>
> > +
> > + if (!num_ports)
> > + return -EINVAL;
> > +
> > + /*
> > + * Adjust gc->max_num_queues returned from the SOC to allow
> dedicated
> > + * MSIx for each vPort. Clamp to no less than
> MANA_DEF_NUM_QUEUES.
> > + */
> > + max_num_queues = (gc->num_msix_usable - 1) / num_ports;
> > + max_num_queues = roundup_pow_of_two(max(max_num_queues, 1U));
> > + if (max_num_queues < MANA_DEF_NUM_QUEUES)
> > + max_num_queues = MANA_DEF_NUM_QUEUES;
> > +
> > + /*
> > + * Use dedicated MSIx for EQs whenever possible, use MSIx sharing for
> > + * Ethernet EQs when (max_num_queues * num_ports >
> num_msix_usable - 1)
> > + */
> > + max_num_queues = min(gc->max_num_queues, max_num_queues);
> > + if (max_num_queues * num_ports > gc->num_msix_usable - 1)
> > + gc->msi_sharing = true;
>
> Is gc->msi_sharing ever reset to false? The only two writers are this line and
> mana_gd_setup_hwc_irqs() (the !pci_msix_can_alloc_dyn branch), and both only
> set it to true. mana_gd_remove_irqs() frees msi_bitmap and zeros
> max_num_msix / num_msix_usable, but does not clear msi_sharing, and the
> gdma_context survives mana_gd_suspend() / mana_gd_resume().
>
> Once true is latched in any setup cycle, mana_gd_setup() will always take the
> sharing branch on resume even if the recomputed max_num_queues * num_ports
> fits within num_msix_usable - 1. Is that consistent with the commit message
> saying sharing "is only enabled when there are not enough MSI-X vectors for
> dedicated allocation"?
msi_sharing is one-directional by design, but this is not a problem in practice. The inputs to the sharing computation - num_msix_usable, num_ports, and hardware queue limits - are properties of
the hardware and do not change across suspend/resume cycles. The gdma_context is kzalloc'd at probe time (so msi_sharing starts as false), and on any given hardware configuration the computation
in mana_gd_query_max_resources() will always produce the same result.
On resume, mana_gd_setup_hwc_irqs() re-evaluates dynamic MSI-X support and mana_gd_query_max_resources() recomputes the sharing check from the same hardware parameters. Since the hardware
doesn't change, the flag converges to the same value. Adding an explicit reset would be defensive but not functionally necessary.
>
> > +
> > + /* If MSI is shared, use max allowed value */
> > + if (gc->msi_sharing)
> > + gc->max_num_queues_vport = min(gc->num_msix_usable - 1,
> gc->max_num_queues);
> > + else
> > + gc->max_num_queues_vport = max_num_queues;
> > +
> > + dev_info(gc->dev, "MSI sharing mode %d max queues %d\n",
> > + gc->msi_sharing, gc->max_num_queues);
>
> Should this print gc->max_num_queues_vport rather than
> gc->max_num_queues? The block immediately above computes
> gc->max_num_queues_vport and leaves gc->max_num_queues unchanged, and
> mana_probe_port() sizes alloc_etherdev_mq() and apc->max_queues from
> gc->max_num_queues_vport:
>
> ndev = alloc_etherdev_mq(sizeof(struct mana_port_context),
> gc->max_num_queues_vport);
> ...
> apc->max_queues = gc->max_num_queues_vport;
> apc->num_queues = min(gc->max_num_queues_vport,
> MANA_DEF_NUM_QUEUES);
>
> so the logged value does not match the per-vPort queue count the driver actually
> exposes.
The printed value is intentional. gc->max_num_queues is the GDMA device-level maximum, which is consumed by both the Ethernet and RDMA drivers - the RDMA driver uses it to size its queue
resources. Printing max_num_queues_vport here would only reflect the Ethernet per-vPort limit and would be misleading in a device-level log message. The per-vPort value is already visible
through ethtool channel reporting.
>
> > +
> > return 0;
> > }
> >
> > @@ -1856,6 +1893,7 @@ static int mana_gd_setup_hwc_irqs(struct pci_dev
> > *pdev)
>
> [ ... ]
>
> > @@ -1968,20 +2008,30 @@ static int mana_gd_setup(struct pci_dev *pdev)
> > if (err)
> > goto destroy_hwc;
> >
> > - err = mana_gd_query_max_resources(pdev);
> > + err = mana_gd_detect_devices(pdev);
> > if (err)
> > goto destroy_hwc;
> >
> > - err = mana_gd_setup_remaining_irqs(pdev);
> > - if (err) {
> > - dev_err(gc->dev, "Failed to setup remaining IRQs: %d", err);
> > - goto destroy_hwc;
> > - }
> > -
> > - err = mana_gd_detect_devices(pdev);
> > + err = mana_gd_query_max_resources(pdev);
> > if (err)
> > goto destroy_hwc;
> >
> > + if (!gc->msi_sharing) {
> > + gc->msi_bitmap = bitmap_zalloc(gc->num_msix_usable,
> GFP_KERNEL);
> > + if (!gc->msi_bitmap) {
> > + err = -ENOMEM;
> > + goto destroy_hwc;
> > + }
> > + /* Set bit for HWC */
> > + set_bit(0, gc->msi_bitmap);
> > + } else {
> > + err = mana_gd_setup_remaining_irqs(pdev);
> > + if (err) {
> > + dev_err(gc->dev, "Failed to setup remaining IRQs: %d",
> err);
> > + goto destroy_hwc;
> > + }
> > + }
> > +
>
> Can the driver bring up any vPort after this change when the !msi_sharing branch
> is taken?
>
> In the dedicated branch, only gc->msi_bitmap is allocated and bit 0 is set for
> HWC. mana_gd_setup_remaining_irqs() is skipped, so no gdma_irq_context is
> inserted into gc->irq_contexts for indices 1..
> num_msix_usable-1.
>
> Later, mana_create_eq() still assigns
>
> spec.eq.msix_index = (i + 1) % gc->num_msix_usable;
>
> and mana_gd_register_irq() does:
>
> gic = xa_load(&gc->irq_contexts, msi_index);
> if (WARN_ON(!gic))
> return -EINVAL;
>
> On a typical cloud SKU with, for example, num_msix_usable=32,
> num_ports=1 and num_online_cpus=16, the new math keeps msi_sharing=false
> (16 * 1 <= 31), so every EQ-create goes down this path and hits the WARN_ON.
> Doesn't that make every vPort open and every resume fail for the common
> dedicated-MSI-X case?
>
> The msi_bitmap allocated here is not consumed anywhere in this commit; the on-
> demand allocation via mana_gd_get_gic() appears in the later commit "net:
> mana: Allocate interrupt context for each EQ when creating vPort"
> (dbbdf40a8974). Should the bitmap and the new branch be introduced in the
> same commit that actually uses them, so each commit in the series is
> independently bootable?
You're right that the non-sharing EQ creation path is not fully functional until patch 5 wires mana_gd_get_gic() into mana_create_eq(). However, this is a new capability being built
incrementally: patch 2 introduces the decision framework and bitmap, patch 3 adds the GIC infrastructure, patch 4 converts global IRQ setup to use it, and patch 5 integrates it into per-vPort EQ
creation.
The intermediate state between patches 2 and 5 results in a clean error (-EINVAL from mana_gd_register_irq) - not a crash or data corruption. The dedicated MSI-X mode is a new feature that did
not exist before this series, so there is no regression from the pre-patch baseline. Restructuring to make it functional at each intermediate commit would require squashing the GIC
infrastructure into this patch, producing a significantly larger and harder-to-review change. I'd prefer to keep the logical separation as-is.
^ permalink raw reply
* [PATCH net v1] net/mlx5e: Fix PTP TX SQ cleanup on metadata DB failure
From: Prathamesh Deshpande @ 2026-05-04 22:30 UTC (permalink / raw)
To: Saeed Mahameed, Leon Romanovsky
Cc: Richard Cochran, Tariq Toukan, Eran Ben Elisha, Jakub Kicinski,
netdev, linux-kernel, Prathamesh Deshpande
mlx5e_ptp_open_txqsq() creates the hardware SQ before allocating the PTP
traffic metadata database.
If mlx5e_ptp_alloc_traffic_db() fails, the error path frees the software
TX queue state but skips destroying the already-created hardware SQ.
Add a dedicated unwind label that destroys the SQ before freeing the TXQ
state.
Fixes: 1880bc4e4a96 ("net/mlx5e: Add TX port timestamp support")
Signed-off-by: Prathamesh Deshpande <prathameshdeshpande7@gmail.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
index 723f66a6bd63..45db2dd7408d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
@@ -489,12 +489,14 @@ static int mlx5e_ptp_open_txqsq(struct mlx5e_ptp *c, u32 tisn,
err = mlx5e_ptp_alloc_traffic_db(ptpsq, dev_to_node(mlx5_core_dma_dev(c->mdev)));
if (err)
- goto err_free_txqsq;
+ goto err_destroy_sq;
INIT_WORK(&ptpsq->report_unhealthy_work, mlx5e_ptpsq_unhealthy_work);
return 0;
+err_destroy_sq:
+ mlx5e_ptp_destroy_sq(c->mdev, txqsq->sqn);
err_free_txqsq:
mlx5e_free_txqsq(txqsq);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH iwl-next v11] ice: add support for unmanaged DPLL on E830 NIC
From: Jacob Keller @ 2026-05-04 22:31 UTC (permalink / raw)
To: Arkadiusz Kubalewski, intel-wired-lan
Cc: netdev, anthony.l.nguyen, przemyslaw.kitszel, linux-doc,
linux-kernel, pmenzel, aleksandr.loktionov, horms, grzegorz.nitka,
vgrinber, zoltan.fodor
In-Reply-To: <20260217155808.1209194-1-arkadiusz.kubalewski@intel.com>
On 2/17/2026 7:58 AM, Arkadiusz Kubalewski wrote:
> Hardware variants of E830 may support an unmanaged DPLL where the
> configuration is hardcoded within the hardware and firmware, meaning
> users cannot modify settings. However, users are able to check the DPLL
> lock status and obtain configuration information through the Linux DPLL
> and devlink health subsystem.
>
> Availability of 'loss of lock' health status code determines if such
> support is available, if true, register single DPLL device with 1 input
> and 1 output and provide hardcoded/read only properties of a pin and
> DPLL device. User is only allowed to check DPLL device status and receive
> notifications on DPLL lock status change.
>
> When present, the DPLL device locks to an external signal provided
> through the PCIe/OCP pin. The expected input signal is 1PPS
> (1 Pulse Per Second) embedded on a 10MHz reference clock.
> The DPLL produces output:
> - for MAC (Media Access Control) & PHY (Physical Layer) clocks,
> - 1PPS for synchronization of onboard PHC (Precision Hardware Clock) timer.
>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> ---
> v11:
> - rebase and fix conflicts
Jakub rightfully pointed out the mistaken inclusion of HAVE_DPLL_ESYNC.
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 73a4e28ed75f..64518202dfff 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> @@ -2588,6 +2601,21 @@ static const struct dpll_pin_ops ice_dpll_output_ops = {
> .esync_get = ice_dpll_output_esync_get,
> };
>
> +static const struct dpll_pin_ops ice_dpll_input_unmanaged_ops = {
> + .frequency_get = ice_dpll_input_frequency_get,
> + .direction_get = ice_dpll_input_direction,
> + .state_on_dpll_get = ice_dpll_input_state_get,
> +#if defined(HAVE_DPLL_ESYNC)
> + .esync_get = ice_dpll_input_esync_get,
> +#endif /* HAVE_DPLL_ESYNC */
> +};
> +
Please fix this, and remove any other remaining kcompat cruft as well.
Also could you please review the following Sashiko reports and make sure
there is no issues?
The netdev sashiko.dev bot says this:
> commit 7aae4a54d3ef18d8694bc535b6e5959839c9adfd
> Author: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> ice: add support for unmanaged DPLL on E830 NIC
> Hardware variants of E830 may support an unmanaged DPLL where the
> configuration is hardcoded. This patch allows users to check the DPLL
> lock status and obtain configuration information through the Linux DPLL
> and devlink health subsystem.
>> diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c b/drivers/net/ethernet/intel/ice/devlink/health.c
>> --- a/drivers/net/ethernet/intel/ice/devlink/health.c
>> +++ b/drivers/net/ethernet/intel/ice/devlink/health.c
> [ ... ]
>> @@ -242,6 +244,8 @@ void ice_process_health_status_event(struct ice_pf *pf, struct ice_rq_event_info
>> pf->health_reporters.fw_status = *health_info;
>> devlink_health_report(pf->health_reporters.fw,
>> "FW syndrome reported", NULL);
>> + if (status_code == ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK)
>> + ice_dpll_lock_state_set_unmanaged(pf, health_info, true);
> Is it possible for this health event to arrive when the DPLL subsystem is not
> fully initialized or already torn down?
> Looking at ice_dpll_lock_state_set_unmanaged(), it unconditionally acquires
> pf->dplls.lock:
> void ice_dpll_lock_state_set_unmanaged(...)
> {
> ...
> mutex_lock(&pf->dplls.lock);
> ...
> }
> If the hardware doesn't support DPLL, ice_dpll_init() is never called, leaving
> the mutex uninitialized. Additionally, if ice_dpll_init_unmanaged() fails,
> it destroys the mutex and exits.
> During driver unload, ice_dpll_deinit() destroys the mutex and frees the
> DPLL structures, but the service task processing these admin queue events
> might not be stopped until later in the teardown sequence.
> Could this lead to a kernel panic from locking an uninitialized or destroyed
> mutex, or a use-after-free when passing the d->dpll pointer to
> dpll_device_change_ntf()?
> Should there be a check ensuring DPLL is active (e.g. by checking
> test_bit(ICE_FLAG_DPLL, pf->flags)) before handling the event?
My own execution using claude had this to say:
> Now I have everything. Let me compose the report.
> commit d9b481c1d5d8be9dbbca36f40be64db91eb12caa
> Author: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
> ice: add support for unmanaged DPLL on E830 NIC
> This patch adds support for an unmanaged DPLL on E830 NIC variants,
> where the DPLL configuration is hardcoded in hardware/firmware. It
> registers a single PPS DPLL device with one input and one output,
> reporting lock status via health status events and the DPLL subsystem.
> A few things stood out during review.
>> diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c b/drivers/net/ethernet/intel/ice/devlink/health.c
>> index 8e9a8a8178d4..31e6c5107c97 100644
>> --- a/drivers/net/ethernet/intel/ice/devlink/health.c
>> +++ b/drivers/net/ethernet/intel/ice/devlink/health.c
> [ ... ]
>> @@ -242,6 +244,8 @@ void ice_process_health_status_event(struct ice_pf *pf, struct ice_rq_event_info
>> pf->health_reporters.fw_status = *health_info;
>> devlink_health_report(pf->health_reporters.fw,
>> "FW syndrome reported", NULL);
>>+ if (status_code == ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK)
>>+ ice_dpll_lock_state_set_unmanaged(pf, health_info, true);
>> break;
> This calls ice_dpll_lock_state_set_unmanaged() for every PF that
> receives a 0x601 health event, with no check for pf->dplls.unmanaged
> or test_bit(ICE_FLAG_DPLL, pf->flags).
> The only guard inside ice_dpll_lock_state_set_unmanaged() is
> ice_pf_src_tmr_owned(), which checks PTP timer ownership -- not
> whether the DPLL subsystem was initialized.
> Consider a device where the firmware supports health code 0x601 but
> the ZL80640 CGU is not in the netlist. In ice_init_features():
>>+ err = ice_is_health_status_code_supported(&pf->hw, code,
>>+ &pf->dplls.unmanaged);
>>+ if (err || !ice_is_unmanaged_cgu_in_netlist(&pf->hw))
>>+ pf->dplls.unmanaged = false;
> This sets pf->dplls.unmanaged = false, so ice_dpll_init() is never
> called and pf->dplls.lock is never initialized via mutex_init().
> But the firmware can still send the health event, and then:
>>+void ice_dpll_lock_state_set_unmanaged(struct ice_pf *pf,
>>+ const struct ice_aqc_health_status_elem *buff,
>>+ bool notify)
>>+{
> [ ... ]
>>+ mutex_lock(&pf->dplls.lock);
> Can this mutex_lock() on an uninitialized mutex cause undefined
> behavior? With CONFIG_DEBUG_MUTEXES this would trigger a BUG().
> Also, on managed DPLL devices (e.g. E810 with CGU), the DPLL worker
> thread maintains pf->dplls.pps.dpll_state via ice_dpll_update_state().
> Would this unconditional call overwrite the managed state machine?
>> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
>> index 3eea83b0cdc9..00704dfeb3b2 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> [ ... ]
>>+static const struct dpll_pin_ops ice_dpll_input_unmanaged_ops = {
>>+ .frequency_get = ice_dpll_input_frequency_get,
>>+ .direction_get = ice_dpll_input_direction,
>>+ .state_on_dpll_get = ice_dpll_input_state_get,
>>+#if defined(HAVE_DPLL_ESYNC)
>>+ .esync_get = ice_dpll_input_esync_get,
>>+#endif /* HAVE_DPLL_ESYNC */
>>+};
> HAVE_DPLL_ESYNC does not appear to be defined anywhere in the upstream
> kernel tree -- it looks like an out-of-tree compatibility macro. In
> upstream builds this means .esync_get is always NULL for unmanaged
> input pins.
> The managed ice_dpll_input_ops sets .esync_get unconditionally, and
> ice_dpll_input_esync_get() already has explicit unmanaged-specific
> logic added by this patch. The init code also unconditionally sets
> ICE_AQC_GET_CGU_IN_CFG_STATUS_ESYNC_CAP and ICE_DPLL_IN_ESYNC_ENABLED
> on unmanaged input pins.
> The documentation example at the end of the patch shows
> esync-frequency and esync-pulse in the pin dump, but those fields
> will never appear in an upstream build since the callback is compiled
> out. Should the ifdef be removed so this matches ice_dpll_input_ops?
> [ ... ]
>> @@ -3153,12 +3181,15 @@ ice_dpll_init_direct_pins(struct ice_pf *pf, bool cgu,
>> int ret;
>>
>> ret = ice_dpll_get_pins(pf, pins, start_idx, count, pf->dplls.clock_id);
>>- if (ret)
>>+ if (!cgu || ret)
>> return ret;
>>- if (cgu) {
>>+
>>+ if (first) {
>> ret = ice_dpll_register_pins(first, pins, ops, count);
>> if (ret)
>> goto release_pins;
>>+ }
>>+ if (second) {
>> ret = ice_dpll_register_pins(second, pins, ops, count);
>> if (ret)
>> goto unregister_first;
> ice_dpll_init_direct_pins() was updated with NULL guards for first
> and second. However, ice_dpll_deinit_direct_pins() was not:
> ice_dpll_deinit_direct_pins() {
> if (cgu) {
> ice_dpll_unregister_pins(first, pins, ops, count);
> ice_dpll_unregister_pins(second, pins, ops, count);
> }
> ...
> }
> For the unmanaged case, ice_dpll_init_unmanaged() only creates the
> PPS dpll -- eec.dpll stays NULL. If output pin init fails inside
> ice_dpll_init_pins(pf, true), the error path falls through to
> deinit_inputs which calls:
> ice_dpll_deinit_direct_pins(pf, cgu=true, ...,
> pf->dplls.pps.dpll,
> pf->dplls.eec.dpll);
> ^^^^
> NULL
> This passes NULL as second to ice_dpll_unregister_pins(), which
> calls dpll_pin_unregister(NULL, ...), dereferencing the NULL dpll
> pointer. Would it make sense to add the same NULL guards to
> ice_dpll_deinit_direct_pins() that were added to the init path?
> [ ... ]
>> @@ -3923,6 +4001,15 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
>> pins[i].prop.board_label = ice_cgu_get_pin_name(hw, i, input);
>> pins[i].prop.type = ice_cgu_get_pin_type(hw, i, input);
>> if (input) {
>>+ if (pf->dplls.unmanaged) {
>>+ pins[i].freq = ice_dpll_pin_freq_info(hw, i,
>>+ input);
>>+ pins[i].state[0] = DPLL_PIN_STATE_CONNECTED;
>>+ pins[i].status =
>>+ ICE_AQC_GET_CGU_IN_CFG_STATUS_ESYNC_CAP;
>>+ pins[i].flags[0] = ICE_DPLL_IN_ESYNC_ENABLED;
>>+ continue;
>>+ }
> [ ... ]
>> } else {
>>+ if (pf->dplls.unmanaged) {
>>+ pins[i].freq = ice_dpll_pin_freq_info(hw, i,
>>+ input);
>>+ pins[i].state[0] = DPLL_PIN_STATE_CONNECTED;
>>+ continue;
>>+ }
> [ ... ]
>> pins[i].prop.freq_supported =
>> ice_cgu_get_pin_freq_supp(hw, i, input, &freq_supp_num);
>> pins[i].prop.freq_supported_num = freq_supp_num;
>> pins[i].pf = pf;
>> }
> Both unmanaged branches use continue, which skips the assignments of
> pins[i].prop.freq_supported, pins[i].prop.freq_supported_num, and
> pins[i].pf at the bottom of the loop.
> The missing freq_supported / freq_supported_num means the DPLL netlink
> code cannot report supported frequency ranges to userspace for
> unmanaged pins, even though the data is available from
> ice_cgu_get_pin_freq_supp().
> The missing pins[i].pf = pf assignment leaves the pointer NULL (from
> kcalloc zeroing). Current unmanaged callbacks use d->pf from
> dpll_priv so there is no immediate crash, but if any callback that
> dereferences p->pf were added to the unmanaged ops in the future it
> would hit a NULL pointer dereference. Was the continue intended to
> skip all of these assignments?
^ permalink raw reply
* Re: [PATCH net-next 07/15] ixgbe: E610: add ACI dynamic debug
From: Jacob Keller @ 2026-05-04 22:33 UTC (permalink / raw)
To: Piotr Kwapulinski
Cc: przemyslaw.kitszel, andrew+netdev, davem, edumazet, pabeni,
aleksander.lobakin, horms, michal.swiatkowski, jedrzej.jagielski,
aleksandr.loktionov, grzegorz.nitka, ivecera,
arkadiusz.kubalewski, anthony.l.nguyen, stefan.wegrzyn,
piotr.kwapulinski, netdev, linux-kernel, pmenzel,
sunithax.d.mekala
In-Reply-To: <20260503020636.4114758-1-kuba@kernel.org>
On 5/2/2026 7:06 PM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ixgbe: E610: add ACI dynamic debug
>
> This commit enables dynamic debugging of the Admin Command Interface (ACI)
> for the e610 adapter, allowing ACI commands and descriptor details to be
> printed into the kernel log.
>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>
> [ ... ]
>
>> @@ -106,8 +185,12 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>> if (desc->flags & cpu_to_le16(LIBIE_AQ_FLAG_RD)) {
>> for (i = 0; i < buf_size / 4; i++)
>> IXGBE_WRITE_REG(hw, IXGBE_PF_HIBA(i), ((u32 *)buf)[i]);
>> - if (buf_tail_size)
>> + ixgbe_aci_debug(hw, desc, buf, buf_size);
>> + if (buf_tail_size) {
>> IXGBE_WRITE_REG(hw, IXGBE_PF_HIBA(i), buf_tail);
>> + ixgbe_aci_debug(hw, desc, &buf_tail,
>> + buf_tail_size);
>> + }
>> }
>> }
>
> Does this result in redundant output being printed to the kernel log?
>
> When buf_tail_size is non-zero, ixgbe_aci_debug() is called twice: once for
> the full buffer, and a second time for just the tail. Since ixgbe_aci_debug()
> prints the full CQ CMD descriptor details each time, this appears to print the
> descriptor headers twice and fragment the buffer output.
>
> [ ... ]
>
>> @@ -183,10 +278,13 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>> if (valid_buf) {
>> for (i = 0; i < buf_size / 4; i++)
>> ((u32 *)buf)[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
>> + ixgbe_aci_debug(hw, raw_desc, buf, buf_size);
>> if (buf_tail_size) {
>> buf_tail = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
>> memcpy(buf + buf_size - buf_tail_size, &buf_tail,
>> buf_tail_size);
>> + ixgbe_aci_debug(hw, raw_desc, &buf_tail,
>> + buf_tail_size);
>> }
>> }
>
> Can this read uninitialized memory from the buffer?
>
> At the point ixgbe_aci_debug(..., buf, buf_size) is called, the loop has only
> populated the 4-byte aligned chunks of the buffer. The trailing bytes at the
> end of the buffer are not populated until the memcpy() occurs later in the
> block. Because ixgbe_aci_debug() consumes buf_size bytes, it looks like it
> will read and log uninitialized trailing memory (potentially from the kernel
> stack).
>
> Also, similar to the transmit path, this calls ixgbe_aci_debug() a second
> time on &buf_tail, which will print the descriptor headers again.
>
> Could we move a single ixgbe_aci_debug(..., buf, buf_size) call to the end of
> the block, after the entire buffer including the tail has been fully written?
Piotr,
Please review this report and submit a new version of this patch to
Intel Wired LAN.
While you are at it, please review the following report from a run of
sashiko I did myself using claude:
> commit 198ec0389c757125d9079647a6c8aba34ba29048
> Author: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
> ixgbe: E610: add ACI dynamic debug
> This patch enables dynamic debug of the Admin Command Interface (ACI)
> for the E610 adapter, adding descriptor and buffer hex dumps via the
> standard dyndbg infrastructure, plus hw_dbg() calls on error paths.
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> index 665a9813e251..b686636beb93 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> [ ... ]
>> @@ -147,6 +230,7 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>> raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA(i));
>> raw_desc[i] = raw_desc[i];
>> }
>>+ ixgbe_aci_debug(hw, raw_desc, NULL, 0);
>> }
> [ ... ]
>> /* Handle timeout and invalid state of HICR register */
>>- if (hicr & IXGBE_PF_HICR_C)
>>+ if (hicr & IXGBE_PF_HICR_C) {
>>+ hw_dbg(hw, "error: Admin Command 0x%X command timeout\n",
>>+ le16_to_cpu(desc->opcode));
>> return -ETIME;
>>+ }
> Since raw_desc is declared as a cast of desc:
> u32 *raw_desc = (u32 *)desc;
> and the sync response read loop overwrites raw_desc[] (and thus desc)
> with firmware response data, could le16_to_cpu(desc->opcode) here
> print the response opcode rather than the original command opcode?
> The local variable opcode already holds the original value saved
> earlier via:
> opcode = le16_to_cpu(desc->opcode);
> Would it be more correct to use opcode directly in this hw_dbg() call
> (and the one in the invalid-state check below)?
>> @@ -183,10 +278,13 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>> if (valid_buf) {
>> for (i = 0; i < buf_size / 4; i++)
>> ((u32 *)buf)[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
>>+ ixgbe_aci_debug(hw, raw_desc, buf, buf_size);
>> if (buf_tail_size) {
>> buf_tail = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
>> memcpy(buf + buf_size - buf_tail_size, &buf_tail,
>> buf_tail_size);
>>+ ixgbe_aci_debug(hw, raw_desc, &buf_tail,
>>+ buf_tail_size);
>> }
>> }
> When buf_size is not 4-byte aligned, ixgbe_aci_debug() is called
> with the full buf_size before the tail bytes have been read from
> hardware and memcpy'd into buf. The hex dump will show stale content
> for the last 1-3 bytes of the buffer.
> Should the ixgbe_aci_debug() call be moved after the tail memcpy so
> that it dumps the complete response?
^ 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