linux-kernel - Re: [PATCH net v2 0/5] Make TCP-MD5-diag slightly less broken

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: <CANn89iJzwe+Wds=otY-iFL9C9eNFVqGi62q085AehnYa3sET7w@mail.gmail.com>
Date: Fri, 6 Dec 2024 16:14:58 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Dmitry Safonov <0x7f454c46@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, 
	Dmitry Safonov via B4 Relay <devnull+0x7f454c46.gmail.com@...nel.org>, 
	"David S. Miller" <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	David Ahern <dsahern@...nel.org>, Ivan Delalande <colona@...sta.com>, 
	Matthieu Baerts <matttbe@...nel.org>, Mat Martineau <martineau@...nel.org>, 
	Geliang Tang <geliang@...nel.org>, Boris Pismenny <borisp@...dia.com>, 
	John Fastabend <john.fastabend@...il.com>, Davide Caratti <dcaratti@...hat.com>, 
	Kuniyuki Iwashima <kuniyu@...zon.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	mptcp@...ts.linux.dev, stable@...r.kernel.org
Subject: Re: [PATCH net v2 0/5] Make TCP-MD5-diag slightly less broken

On Fri, Dec 6, 2024 at 3:49 AM Dmitry Safonov <0x7f454c46@...il.com> wrote:
>
> Hi Jakub, Eric,
>
> On Thu, 5 Dec 2024 at 09:09, Eric Dumazet <edumazet@...gle.com> wrote:
> > On Thu, Dec 5, 2024 at 2:13 AM Jakub Kicinski <kuba@...nel.org> wrote:
> > >
> > > Hi Eric!
> > >
> > > This was posted while you were away -- any thoughts or recommendation on
> > > how to address the required nl message size changing? Or other problems
> > > pointed out by Dmitry? My suggestion in the subthread is to re-dump
> > > with a fixed, large buffer on EMSGSIZE, but that's not super clean..
> >
> > Hi Jakub
> >
> > inet_diag_dump_one_icsk() could retry, doubling the size until the
> > ~32768 byte limit is reached ?
> >
> > Also, we could make sure inet_sk_attr_size() returns at least
> > NLMSG_DEFAULT_SIZE, there is no
> > point trying to save memory for a single skb in inet_diag_dump_one_icsk().
>
> Starting from NLMSG_DEFAULT_SIZE sounds like a really sane idea! :-)

There is a consensus for this one, I will cook a patch with this part only.

>
> [..]
> > @@ -585,8 +589,14 @@ int inet_diag_dump_one_icsk(struct inet_hashinfo *hashinfo,
> >
> >         err = sk_diag_fill(sk, rep, cb, req, 0, net_admin);
> >         if (err < 0) {
> > -               WARN_ON(err == -EMSGSIZE);
> >                 nlmsg_free(rep);
> > +               if (err == -EMSGSIZE) {
> > +                       attr_size <<= 1;
> > +                       if (attr_size + NLMSG_HDRLEN <=
> > SKB_WITH_OVERHEAD(32768)) {
> > +                               cond_resched();
> > +                               goto retry;
> > +                       }
> > +               }
> >                 goto out;
> >         }
> >         err = nlmsg_unicast(net->diag_nlsk, rep, NETLINK_CB(in_skb).portid);
>
> To my personal taste on larger than 327 md5 keys scale, I'd prefer to
> see "dump may be inconsistent, retry if you need consistency" than
> -EMSGSIZE fail, yet userspace potentially may use the errno as a
> "retry" signal.
>

I do not yet understand this point. I will let you send a patch for
further discussion.

Thanks.