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

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2024-11-20 at 16:13 +0000, Dmitry Safonov wrote:
> > We have min_dump_alloc, which a number of places are setting much higher
> > than the default, partially at least because there were similar issues,
> > e.g. in nl80211. See e.g. nl80211_dump_wiphy() doing it dynamically.
> 
> Yeah, your example seems alike what netlink_dump() does with
> min_dump_alloc and max_recvmsg_len. You have there
> .doit = nl80211_get_wiphy,
> .dumpit = nl80211_dump_wiphy,
> 
> So at this initial patch set, I'm trying to fix
> inet_diag_handler::dump_one() callback, which is to my understanding
> same as .doit() for generic netlink [should we just rename struct
> inet_diag_handler callbacks to match the generics?].

dump_one() doesn't sound like doit(), it sounds more like dump one
object? In generic netlink dumpit has to handle that internally, and
doit() is for commands, without F_DUMP.

But also generic netlink is just one netlink family, so I wouldn't
really rename anything to match it anyway.

>  See
> inet_diag_handler_cmd() and NLM_F_DUMP in
> Documentation/userspace-api/netlink/intro.rst
> For TCP-MD5-diag even the single message reply may have a variable
> number of keys on a socket's dump.

Right.

> For multi-messages dump, my plan is
> to use netlink_callback::ctx[] and add an iterator type that will
> allow to stop on N-th key between recvmsg() calls.

Right, so userspace has to understand that format. In nl80211 we've made
an input flag attribute (inputs are often unused with dump, but are
present) to request that split format.

> > Kind of ugly? Sure! And we shouldn't use it now with newer userspace
> > that knows to request a more finely split dump. For older userspace it's
> > the only way though.
> 
> Heh, the comment in nl80211_dump_wiphy() on sending an empty message
> and retrying is ouch!

Yeah ... Luckily we basically converted all userspace to request split
dumps, so we shouldn't ever get there now.

> > For auto-grow you'd also have to have information about the userspace
> > buffer, I think? It still has to fit there, might as well fail anyway if
> > that buffer is too small? I'm not sure we have that link back? But I'm
> > not really sure right now, just remember this as an additional wrinkle
> > from the above-mentioned nl80211 problem.
> 
> Yeah, netlink_recvmsg() attempts to track what the userspace is asking:
> 
> :        /* Record the max length of recvmsg() calls for future allocations */
> :        max_recvmsg_len = max(READ_ONCE(nlk->max_recvmsg_len), len);
> :        max_recvmsg_len = min_t(size_t, max_recvmsg_len,
> :                                SKB_WITH_OVERHEAD(32768));
> :        WRITE_ONCE(nlk->max_recvmsg_len, max_recvmsg_len);

Right, OK, so that's sorted then :)

johannes