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

[Dmitry Safonov]

Hi Jakub, Eric,

On Thu, 5 Dec 2024 at 09:09, Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> On Thu, Dec 5, 2024 at 2:13 AM Jakub Kicinski <kuba@xxxxxxxxxx> 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! :-)

[..]
> @@ -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.

Do you plan to re-send it as a proper patch? Or I can send it with my
next patches for TCP-MD5-diag issues (1), (3), (4) and TCP-AO-diag.

Thanks,
             Dmitry