Re: [PATCH v3 3/3] batman-adv: bcast: remove remaining skb-copy calls for broadcasts

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

On Monday, 17 May 2021 00:33:09 CEST Linus Lüssing wrote:
[...]
> However
> after (queueing for) forwarding the packet in
> batadv_recv_bcast_packet()->batadv_forw_bcast_packet() a packet is
> additionally decapsulated and is sent up the stack through
> batadv_recv_bcast_packet()->batadv_interface_rx(). Which needs an
> unshared skb data for potential modifications from other protocols.
> To be able to use skb clones for (re)broadcasted batman-adv broadcast
> packets while still ensuring that interface_rx() has a freely writeable
> skb we use skb_cow() to avoid sharing skb data with the skb clones in

So you are talking here about netif_rx. But this doesn't seem to ensure that 
the data is "private" at all. It can even easily happen that there is a 
tcpdump listening at the same time on the interface which is "netif_rx"ing. In 
this case, both AF_PACKET and whatever is parsing the layer 3 stuff is 
receiving an skb_shared() skb.

In this case, the receiver of the skb must use skb_share_check to clone the 
skb - so we would end up with the same situation as you had before your 
skb_cow. And it must then for example use skb_cow_data to "gain" write access 
to the skb's data.


Take for example the bridge code. You can find the skb_share_check in 
br_handle_frame. Afterwards, it knows that it has a clone of the skb (but not 
necessarily a private copy of the actual data). If it needs to modify the data 
then it is copying the skb.

Another example is the IPv4 code. One of the first things it does is to check 
in ip_rcv_core for the shared skb. And if it needs to modify it (for example 
by forwarding it in ip_forward), it uses skb_cow directly.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]