From: Kuniyuki Iwashima <kuniyu@amazon.com>
To: <devnull+0x7f454c46.gmail.com@kernel.org>
Cc: <0x7f454c46@gmail.com>, <borisp@nvidia.com>, <colona@arista.com>,
<davem@davemloft.net>, <dsahern@kernel.org>,
<edumazet@google.com>, <geliang@kernel.org>, <horms@kernel.org>,
<john.fastabend@gmail.com>, <kuba@kernel.org>,
<linux-kernel@vger.kernel.org>, <martineau@kernel.org>,
<matttbe@kernel.org>, <mptcp@lists.linux.dev>,
<netdev@vger.kernel.org>, <pabeni@redhat.com>
Subject: Re: [PATCH net 4/6] net/diag: Always pre-allocate tcp_ulp info
Date: Wed, 6 Nov 2024 16:21:33 -0800 [thread overview]
Message-ID: <20241107002133.56595-1-kuniyu@amazon.com> (raw)
In-Reply-To: <20241106-tcp-md5-diag-prep-v1-4-d62debf3dded@gmail.com>
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.
> 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
>
next prev parent reply other threads:[~2024-11-07 0:21 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-06 18:10 [PATCH net 0/6] Make TCP-MD5-diag slightly less broken Dmitry Safonov via B4 Relay
2024-11-06 18:10 ` [PATCH net 1/6] net/diag: Do not race on dumping MD5 keys with adding new MD5 keys Dmitry Safonov via B4 Relay
2024-11-07 4:21 ` kernel test robot
2024-11-06 18:10 ` [PATCH net 2/6] net/diag: Warn only once on EMSGSIZE Dmitry Safonov via B4 Relay
2024-11-06 18:10 ` [PATCH net 3/6] net/diag: Pre-allocate optional info only if requested Dmitry Safonov via B4 Relay
2024-11-06 18:10 ` [PATCH net 4/6] net/diag: Always pre-allocate tcp_ulp info Dmitry Safonov via B4 Relay
2024-11-07 0:21 ` Kuniyuki Iwashima [this message]
2024-11-07 17:35 ` Dmitry Safonov
2024-11-06 18:10 ` [PATCH net 5/6] net/diag: Limit TCP-MD5-diag array by max attribute length Dmitry Safonov via B4 Relay
2024-11-07 0:25 ` Kuniyuki Iwashima
2024-11-07 17:46 ` Dmitry Safonov
2024-11-06 18:10 ` [PATCH net 6/6] net/netlink: Correct the comment on netlink message max cap Dmitry Safonov via B4 Relay
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241107002133.56595-1-kuniyu@amazon.com \
--to=kuniyu@amazon.com \
--cc=0x7f454c46@gmail.com \
--cc=borisp@nvidia.com \
--cc=colona@arista.com \
--cc=davem@davemloft.net \
--cc=devnull+0x7f454c46.gmail.com@kernel.org \
--cc=dsahern@kernel.org \
--cc=edumazet@google.com \
--cc=geliang@kernel.org \
--cc=horms@kernel.org \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=martineau@kernel.org \
--cc=matttbe@kernel.org \
--cc=mptcp@lists.linux.dev \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).