linux-kernel - Re: [PATCH net 4/6] net/diag: Always pre-allocate tcp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJwJo6bnYm9cuJ=-xXw4n5dscq+CG4mpbtOc_vLMFmWKD2GD1g@mail.gmail.com>
Date: Thu, 7 Nov 2024 17:35:53 +0000
From: Dmitry Safonov <0x7f454c46@...il.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: devnull+0x7f454c46.gmail.com@...nel.org, borisp@...dia.com, 
	colona@...sta.com, davem@...emloft.net, dsahern@...nel.org, 
	edumazet@...gle.com, geliang@...nel.org, horms@...nel.org, 
	john.fastabend@...il.com, kuba@...nel.org, linux-kernel@...r.kernel.org, 
	martineau@...nel.org, matttbe@...nel.org, mptcp@...ts.linux.dev, 
	netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net 4/6] net/diag: Always pre-allocate tcp_ulp info

Hi Kuniyuki,

thanks for your review,

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