Re: [PATCH net 4/6] net/diag: Always pre-allocate tcp_ulp info

From: Dmitry Safonov
Date: Thu Nov 07 2024 - 12:36:16 EST


Hi Kuniyuki,

thanks for your review,

On Thu, 7 Nov 2024 at 00:21, Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
>
> From: Dmitry Safonov via B4 Relay <devnull+0x7f454c46.gmail.com@xxxxxxxxxx>
> Date: Wed, 06 Nov 2024 18:10:17 +0000
> > From: Dmitry Safonov <0x7f454c46@xxxxxxxxx>
> >
> > 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