Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <fec97dd04fe0b91a2d0d046839344f62cd8e127a.camel@sipsolutions.net>
Subject: Re: [RFCv1 2/2] nl80211: Don't split-dump for clients with large
 buffers
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Denis Kenzior <denkenz@gmail.com>, linux-wireless@vger.kernel.org
Date:   Thu, 01 Aug 2019 11:13:55 +0200
In-Reply-To: <20190801071455.4974-2-denkenz@gmail.com> (sfid-20190801_091504_260738_7DFFEF78)
References: <20190801071455.4974-1-denkenz@gmail.com>
         <20190801071455.4974-2-denkenz@gmail.com>
         (sfid-20190801_091504_260738_7DFFEF78)
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.30.5 (3.30.5-1.fc29) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Thu, 2019-08-01 at 02:14 -0500, Denis Kenzior wrote:

> +		/*
> +		 * auto-detect support for large buffer sizes: af_netlink
> +		 * will allocate skbufs larger than 4096 in cases where
> +		 * it detects that the client receive buffer (given to
> +		 * recvmsg) is bigger.  In such cases we can assume that
> +		 * performing split dumps is wasteful since the client
> +		 * can likely safely consume the entire un-split wiphy
> +		 * message in one go without the extra message header
> +		 * overhead.
> +		 */
> +		if (skb_tailroom(skb) > 4096) {
> +			state->large_message = true;
> +			state->split = false;
> +		}

Hmm. That's kinda a neat idea, but I don't think it's a good idea. Have
you checked how long the message is now?

Since we *did* in fact hit the previous limit, and have added a *lot* of
things since then (this was years ago, after all), I wouldn't be
surprised if we're reasonably close to the new limit you propose even
now already.

Also, keep in mind that there are some devices that just have an
*enormous* amount of channels, and that's only going to increase (right
now with 6/7 GHz, etc.)

So in general, given all the variable things we have here, all this
buffer size estimation doesn't seem very robust to me. You could have
any number of variable things in a message:
 * channel list - which we alleviated somewhat by having a separate
   channel dump, so not all data is included here (which I guess you'll
   complain about next :P)
 * nl80211_send_mgmt_stypes() things are also a bit variable, and we
   keep adding interface types etc., and some devices may support lots
   of frames (there's an upper bound, but it's not that small)
 * interface combinations - only getting more complex with more complex
   devices and more concurrency use cases
 * vendor commands have no real limit
 * I'm sure measurement use cases will only increases
 * and generally of course we keep adding to everything


Also, I don't really buy the *need* for this since you're just removing
a few kernel/user roundtrips here when new devices are discovered, a
rare event. The parsing isn't really any more complicated for the
userspace side.


Regarding the other patch, I think most of the above also applies there.
I can sort of see how you think it's *nice* to have all the data right
there, but I really don't see why you're so hung up about having to
request the full information ... And I really don't want to see this hit
the wall again in the future, in some weird scenarios with devices that
have lots of <any of the above information>.


> It should be safe to assume that any users of these new unsolicited
> NEW_WIPHY events are non-legacy clients, which can use a
> larger receive buffer for netlink messages.  Since older, legacy clients
> did not utilize NEW_WIPHY events (they did not exist), it is assumed
> that even if the client receives such a message (even if truncated), no
> harm would result and backwards-compatibility would be kept.

Interesting idea, but no, in general you cannot assume that. Older
clients might have added support for NEW_WIPHY without fixing the split
dumps first ...

Also, you mention in the code that messages are truncated, but I'm
pretty sure they're just dropped, not truncated.

And finally, I also see no reason to send out many KB of data for what
might in the end (e.g. in iw) just be a debug message.


But really I think the thing that kills this proposal is the fact that
it reintroduces a message size limit (even if higher now) that we're
somewhat likely to hit in the future.

johannes