From: Dmitry Safonov <0x7f454c46@gmail.com>
Currently there is a theoretical race between netlink one-socket dump
and allocating icsk->icsk_ulp_ops.
Simplify the expectations by always allocating maximum tcp_ulp-info.
With the previous patch the typical netlink message allocation was
decreased for kernel replies on requests without idiag_ext flags,
so let's use it.
Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
---
include/net/tcp.h | 1 -
net/ipv4/inet_diag.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
net/ipv4/tcp_diag.c | 13 -------------
net/mptcp/diag.c | 20 --------------------
net/tls/tls_main.c | 17 -----------------
5 files changed, 48 insertions(+), 51 deletions(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index d1948d357dade0842777265d3397842919f9eee0..757711aa5337ae7e6abee62d303eb66d37082e19 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2568,7 +2568,6 @@ struct tcp_ulp_ops {
void (*release)(struct sock *sk);
/* diagnostic */
int (*get_info)(struct sock *sk, struct sk_buff *skb);
- size_t (*get_info_size)(const struct sock *sk);
/* clone ulp */
void (*clone)(const struct request_sock *req, struct sock *newsk,
const gfp_t priority);
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 2dd173a73bd1e2657957e5e4ecb70401cc85dfda..97862971d552216e574cac3dd2a8fc8c893888d3 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -97,6 +97,53 @@ void inet_diag_msg_common_fill(struct inet_diag_msg *r, struct sock *sk)
}
EXPORT_SYMBOL_GPL(inet_diag_msg_common_fill);
+static size_t tls_get_info_size(void)
+{
+ size_t size = 0;
+
+#ifdef CONFIG_TLS
+ size += nla_total_size(0) + /* INET_ULP_INFO_TLS */
+ nla_total_size(sizeof(u16)) + /* TLS_INFO_VERSION */
+ nla_total_size(sizeof(u16)) + /* TLS_INFO_CIPHER */
+ nla_total_size(sizeof(u16)) + /* TLS_INFO_RXCONF */
+ nla_total_size(sizeof(u16)) + /* TLS_INFO_TXCONF */
+ nla_total_size(0) + /* TLS_INFO_ZC_RO_TX */
+ nla_total_size(0) + /* TLS_INFO_RX_NO_PAD */
+ 0;
+#endif
+
+ return size;
+}
+
+static size_t subflow_get_info_size(void)
+{
+ size_t size = 0;
+
+#ifdef CONFIG_MPTCP
+ size += nla_total_size(0) + /* INET_ULP_INFO_MPTCP */
+ nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_TOKEN_REM */
+ nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_TOKEN_LOC */
+ nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_RELWRITE_SEQ */
+ nla_total_size_64bit(8) + /* MPTCP_SUBFLOW_ATTR_MAP_SEQ */
+ nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_MAP_SFSEQ */
+ nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_SSN_OFFSET */
+ nla_total_size(2) + /* MPTCP_SUBFLOW_ATTR_MAP_DATALEN */
+ nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_FLAGS */
+ nla_total_size(1) + /* MPTCP_SUBFLOW_ATTR_ID_REM */
+ nla_total_size(1) + /* MPTCP_SUBFLOW_ATTR_ID_LOC */
+ 0;
+#endif
+
+ return size;
+}
+
+static size_t tcp_ulp_ops_size(void)
+{
+ size_t size = max(tls_get_info_size(), subflow_get_info_size());
+
+ return size + nla_total_size(0) + nla_total_size(TCP_ULP_NAME_MAX);
+}
+
static size_t inet_sk_attr_size(struct sock *sk,
const struct inet_diag_req_v2 *req,
bool net_admin)
@@ -115,6 +162,7 @@ static size_t inet_sk_attr_size(struct sock *sk,
ret += nla_total_size(sizeof(struct tcp_info))
+ nla_total_size(sizeof(struct inet_diag_msg))
+ inet_diag_msg_attrs_size()
+ + tcp_ulp_ops_size()
+ 64;
if (ext & (1 << (INET_DIAG_MEMINFO - 1)))
diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
index 36606a19b451f059e32c58c0d76a878dc9be5ff0..722dbfd54d247b4def1e77b1674c5b207c5a939d 100644
--- a/net/ipv4/tcp_diag.c
+++ b/net/ipv4/tcp_diag.c
@@ -154,7 +154,6 @@ static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
{
- struct inet_connection_sock *icsk = inet_csk(sk);
size_t size = 0;
#ifdef CONFIG_TCP_MD5SIG
@@ -174,18 +173,6 @@ static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
sizeof(struct tcp_diag_md5sig));
}
#endif
-
- if (net_admin && sk_fullsock(sk)) {
- const struct tcp_ulp_ops *ulp_ops;
-
- ulp_ops = icsk->icsk_ulp_ops;
- if (ulp_ops) {
- size += nla_total_size(0) +
- nla_total_size(TCP_ULP_NAME_MAX);
- if (ulp_ops->get_info_size)
- size += ulp_ops->get_info_size(sk);
- }
- }
return size;
}
diff --git a/net/mptcp/diag.c b/net/mptcp/diag.c
index 2d3efb405437d85c0bca70d7a92ca3a7363365e1..8b36867e4ddd5f45cebcf60e9093a061d5208756 100644
--- a/net/mptcp/diag.c
+++ b/net/mptcp/diag.c
@@ -84,27 +84,7 @@ static int subflow_get_info(struct sock *sk, struct sk_buff *skb)
return err;
}
-static size_t subflow_get_info_size(const struct sock *sk)
-{
- size_t size = 0;
-
- size += nla_total_size(0) + /* INET_ULP_INFO_MPTCP */
- nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_TOKEN_REM */
- nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_TOKEN_LOC */
- nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_RELWRITE_SEQ */
- nla_total_size_64bit(8) + /* MPTCP_SUBFLOW_ATTR_MAP_SEQ */
- nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_MAP_SFSEQ */
- nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_SSN_OFFSET */
- nla_total_size(2) + /* MPTCP_SUBFLOW_ATTR_MAP_DATALEN */
- nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_FLAGS */
- nla_total_size(1) + /* MPTCP_SUBFLOW_ATTR_ID_REM */
- nla_total_size(1) + /* MPTCP_SUBFLOW_ATTR_ID_LOC */
- 0;
- return size;
-}
-
void mptcp_diag_subflow_init(struct tcp_ulp_ops *ops)
{
ops->get_info = subflow_get_info;
- ops->get_info_size = subflow_get_info_size;
}
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 6b4b9f2749a6fd6de495940c5cb3f2154a5a451e..f3491c4e942e08dc882cb81eef071203384b2b37 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -1072,22 +1072,6 @@ static int tls_get_info(struct sock *sk, struct sk_buff *skb)
return err;
}
-static size_t tls_get_info_size(const struct sock *sk)
-{
- size_t size = 0;
-
- size += nla_total_size(0) + /* INET_ULP_INFO_TLS */
- nla_total_size(sizeof(u16)) + /* TLS_INFO_VERSION */
- nla_total_size(sizeof(u16)) + /* TLS_INFO_CIPHER */
- nla_total_size(sizeof(u16)) + /* TLS_INFO_RXCONF */
- nla_total_size(sizeof(u16)) + /* TLS_INFO_TXCONF */
- nla_total_size(0) + /* TLS_INFO_ZC_RO_TX */
- nla_total_size(0) + /* TLS_INFO_RX_NO_PAD */
- 0;
-
- return size;
-}
-
static int __net_init tls_init_net(struct net *net)
{
int err;
@@ -1123,7 +1107,6 @@ static struct tcp_ulp_ops tcp_tls_ulp_ops __read_mostly = {
.init = tls_init,
.update = tls_update,
.get_info = tls_get_info,
- .get_info_size = tls_get_info_size,
};
static int __init tls_register(void)
--
2.42.2From: Dmitry Safonov via B4 Relay <devnull+0x7f454c46.gmail.com@kernel.org>
Date: Wed, 06 Nov 2024 18:10:17 +0000
> From: Dmitry Safonov <0x7f454c46@gmail.com>
>
> Currently there is a theoretical race between netlink one-socket dump
> and allocating icsk->icsk_ulp_ops.
>
> Simplify the expectations by always allocating maximum tcp_ulp-info.
> With the previous patch the typical netlink message allocation was
> decreased for kernel replies on requests without idiag_ext flags,
> so let's use it.
>
I think Fixes tag is needed.
> Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
> ---
> include/net/tcp.h | 1 -
> net/ipv4/inet_diag.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> net/ipv4/tcp_diag.c | 13 -------------
> net/mptcp/diag.c | 20 --------------------
> net/tls/tls_main.c | 17 -----------------
> 5 files changed, 48 insertions(+), 51 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index d1948d357dade0842777265d3397842919f9eee0..757711aa5337ae7e6abee62d303eb66d37082e19 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2568,7 +2568,6 @@ struct tcp_ulp_ops {
> void (*release)(struct sock *sk);
> /* diagnostic */
> int (*get_info)(struct sock *sk, struct sk_buff *skb);
> - size_t (*get_info_size)(const struct sock *sk);
> /* clone ulp */
> void (*clone)(const struct request_sock *req, struct sock *newsk,
> const gfp_t priority);
> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> index 2dd173a73bd1e2657957e5e4ecb70401cc85dfda..97862971d552216e574cac3dd2a8fc8c893888d3 100644
> --- a/net/ipv4/inet_diag.c
> +++ b/net/ipv4/inet_diag.c
> @@ -97,6 +97,53 @@ void inet_diag_msg_common_fill(struct inet_diag_msg *r, struct sock *sk)
> }
> EXPORT_SYMBOL_GPL(inet_diag_msg_common_fill);
>
> +static size_t tls_get_info_size(void)
> +{
> + size_t size = 0;
> +
> +#ifdef CONFIG_TLS
> + size += nla_total_size(0) + /* INET_ULP_INFO_TLS */
It seems '\t' after '+' was converted to '\s' by copy-and-paste.
> + nla_total_size(sizeof(u16)) + /* TLS_INFO_VERSION */
> + nla_total_size(sizeof(u16)) + /* TLS_INFO_CIPHER */
> + nla_total_size(sizeof(u16)) + /* TLS_INFO_RXCONF */
> + nla_total_size(sizeof(u16)) + /* TLS_INFO_TXCONF */
> + nla_total_size(0) + /* TLS_INFO_ZC_RO_TX */
> + nla_total_size(0) + /* TLS_INFO_RX_NO_PAD */
> + 0;
> +#endif
> +
> + return size;
> +}
> +
> +static size_t subflow_get_info_size(void)
> +{
> + size_t size = 0;
> +
> +#ifdef CONFIG_MPTCP
> + size += nla_total_size(0) + /* INET_ULP_INFO_MPTCP */
> + nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_TOKEN_REM */
> + nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_TOKEN_LOC */
> + nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_RELWRITE_SEQ */
> + nla_total_size_64bit(8) + /* MPTCP_SUBFLOW_ATTR_MAP_SEQ */
While at it, let's adjust tabs to match with MPTCP_SUBFLOW_ATTR_MAP_SEQ.
> + nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_MAP_SFSEQ */
> + nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_SSN_OFFSET */
> + nla_total_size(2) + /* MPTCP_SUBFLOW_ATTR_MAP_DATALEN */
> + nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_FLAGS */
> + nla_total_size(1) + /* MPTCP_SUBFLOW_ATTR_ID_REM */
> + nla_total_size(1) + /* MPTCP_SUBFLOW_ATTR_ID_LOC */
> + 0;
> +#endif
> +
> + return size;
> +}
> +
> +static size_t tcp_ulp_ops_size(void)
> +{
> + size_t size = max(tls_get_info_size(), subflow_get_info_size());
> +
> + return size + nla_total_size(0) + nla_total_size(TCP_ULP_NAME_MAX);
Is nla_total_size(0) for INET_DIAG_ULP_INFO ?
It would be better to break them down in the same format with comment
like tls_get_info_size() and subflow_get_info_size().
> +}
> +
> static size_t inet_sk_attr_size(struct sock *sk,
> const struct inet_diag_req_v2 *req,
> bool net_admin)
> @@ -115,6 +162,7 @@ static size_t inet_sk_attr_size(struct sock *sk,
> ret += nla_total_size(sizeof(struct tcp_info))
> + nla_total_size(sizeof(struct inet_diag_msg))
> + inet_diag_msg_attrs_size()
> + + tcp_ulp_ops_size()
> + 64;
>
> if (ext & (1 << (INET_DIAG_MEMINFO - 1)))
> diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
> index 36606a19b451f059e32c58c0d76a878dc9be5ff0..722dbfd54d247b4def1e77b1674c5b207c5a939d 100644
> --- a/net/ipv4/tcp_diag.c
> +++ b/net/ipv4/tcp_diag.c
> @@ -154,7 +154,6 @@ static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
>
> static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
> {
> - struct inet_connection_sock *icsk = inet_csk(sk);
> size_t size = 0;
>
> #ifdef CONFIG_TCP_MD5SIG
> @@ -174,18 +173,6 @@ static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
> sizeof(struct tcp_diag_md5sig));
> }
> #endif
> -
> - if (net_admin && sk_fullsock(sk)) {
> - const struct tcp_ulp_ops *ulp_ops;
> -
> - ulp_ops = icsk->icsk_ulp_ops;
> - if (ulp_ops) {
> - size += nla_total_size(0) +
> - nla_total_size(TCP_ULP_NAME_MAX);
> - if (ulp_ops->get_info_size)
> - size += ulp_ops->get_info_size(sk);
> - }
> - }
> return size;
> }
>
> diff --git a/net/mptcp/diag.c b/net/mptcp/diag.c
> index 2d3efb405437d85c0bca70d7a92ca3a7363365e1..8b36867e4ddd5f45cebcf60e9093a061d5208756 100644
> --- a/net/mptcp/diag.c
> +++ b/net/mptcp/diag.c
> @@ -84,27 +84,7 @@ static int subflow_get_info(struct sock *sk, struct sk_buff *skb)
> return err;
> }
>
> -static size_t subflow_get_info_size(const struct sock *sk)
> -{
> - size_t size = 0;
> -
> - size += nla_total_size(0) + /* INET_ULP_INFO_MPTCP */
> - nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_TOKEN_REM */
> - nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_TOKEN_LOC */
> - nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_RELWRITE_SEQ */
> - nla_total_size_64bit(8) + /* MPTCP_SUBFLOW_ATTR_MAP_SEQ */
> - nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_MAP_SFSEQ */
> - nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_SSN_OFFSET */
> - nla_total_size(2) + /* MPTCP_SUBFLOW_ATTR_MAP_DATALEN */
> - nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_FLAGS */
> - nla_total_size(1) + /* MPTCP_SUBFLOW_ATTR_ID_REM */
> - nla_total_size(1) + /* MPTCP_SUBFLOW_ATTR_ID_LOC */
> - 0;
> - return size;
> -}
> -
> void mptcp_diag_subflow_init(struct tcp_ulp_ops *ops)
> {
> ops->get_info = subflow_get_info;
> - ops->get_info_size = subflow_get_info_size;
> }
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index 6b4b9f2749a6fd6de495940c5cb3f2154a5a451e..f3491c4e942e08dc882cb81eef071203384b2b37 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -1072,22 +1072,6 @@ static int tls_get_info(struct sock *sk, struct sk_buff *skb)
> return err;
> }
>
> -static size_t tls_get_info_size(const struct sock *sk)
> -{
> - size_t size = 0;
> -
> - size += nla_total_size(0) + /* INET_ULP_INFO_TLS */
> - nla_total_size(sizeof(u16)) + /* TLS_INFO_VERSION */
> - nla_total_size(sizeof(u16)) + /* TLS_INFO_CIPHER */
> - nla_total_size(sizeof(u16)) + /* TLS_INFO_RXCONF */
> - nla_total_size(sizeof(u16)) + /* TLS_INFO_TXCONF */
> - nla_total_size(0) + /* TLS_INFO_ZC_RO_TX */
> - nla_total_size(0) + /* TLS_INFO_RX_NO_PAD */
> - 0;
> -
> - return size;
> -}
> -
> static int __net_init tls_init_net(struct net *net)
> {
> int err;
> @@ -1123,7 +1107,6 @@ static struct tcp_ulp_ops tcp_tls_ulp_ops __read_mostly = {
> .init = tls_init,
> .update = tls_update,
> .get_info = tls_get_info,
> - .get_info_size = tls_get_info_size,
> };
>
> static int __init tls_register(void)
>
> --
> 2.42.2
>
Hi Kuniyuki,
thanks for your review,
On Thu, 7 Nov 2024 at 00:21, Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Dmitry Safonov via B4 Relay <devnull+0x7f454c46.gmail.com@kernel.org>
> Date: Wed, 06 Nov 2024 18:10:17 +0000
> > From: Dmitry Safonov <0x7f454c46@gmail.com>
> >
> > Currently there is a theoretical race between netlink one-socket dump
> > and allocating icsk->icsk_ulp_ops.
> >
> > Simplify the expectations by always allocating maximum tcp_ulp-info.
> > With the previous patch the typical netlink message allocation was
> > decreased for kernel replies on requests without idiag_ext flags,
> > so let's use it.
> >
>
> I think Fixes tag is needed.
Yeah, probably, wasn't sure if it's -stable material as I didn't
attempt to create a reproducer for this.
[..]
> > @@ -97,6 +97,53 @@ void inet_diag_msg_common_fill(struct inet_diag_msg *r, struct sock *sk)
> > }
> > EXPORT_SYMBOL_GPL(inet_diag_msg_common_fill);
> >
> > +static size_t tls_get_info_size(void)
> > +{
> > + size_t size = 0;
> > +
> > +#ifdef CONFIG_TLS
> > + size += nla_total_size(0) + /* INET_ULP_INFO_TLS */
>
> It seems '\t' after '+' was converted to '\s' by copy-and-paste.
Thanks, will correct
[..]
> > +static size_t subflow_get_info_size(void)
> > +{
> > + size_t size = 0;
> > +
> > +#ifdef CONFIG_MPTCP
> > + size += nla_total_size(0) + /* INET_ULP_INFO_MPTCP */
> > + nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_TOKEN_REM */
> > + nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_TOKEN_LOC */
> > + nla_total_size(4) + /* MPTCP_SUBFLOW_ATTR_RELWRITE_SEQ */
> > + nla_total_size_64bit(8) + /* MPTCP_SUBFLOW_ATTR_MAP_SEQ */
>
> While at it, let's adjust tabs to match with MPTCP_SUBFLOW_ATTR_MAP_SEQ.
Sure
[..]
> > +static size_t tcp_ulp_ops_size(void)
> > +{
> > + size_t size = max(tls_get_info_size(), subflow_get_info_size());
> > +
> > + return size + nla_total_size(0) + nla_total_size(TCP_ULP_NAME_MAX);
>
> Is nla_total_size(0) for INET_DIAG_ULP_INFO ?
>
> It would be better to break them down in the same format with comment
> like tls_get_info_size() and subflow_get_info_size().
Good idea! Will do in v2.
[..]
Thanks,
Dmitry
© 2016 - 2024 Red Hat, Inc.