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

  We currently have two code paths for broadcast packets: A)
self-generated, via batadv_interface_tx()->batadv_send_bcast_packet().
Or B) received/forwarded, via batadv_recv_bcast_packet()->
batadv_forw_bcast_packet().

For A), self-generated broadcast packets the only modifications to the
skb data is the ethernet header which is added/pushed to the skb in
batadv_send_broadcast_skb()->batadv_send_skb_packet(). However before
doing so batadv_skb_head_push() is called which calls skb_cow_head() to
unshare the space for the to be pushed ethernet header. So for this
case, it is safe to use skb clones.

For B), received/forwarded packets the same applies as in A) for the
to be forwarded packets. Only the ethernet header is added. 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
the forwarding calls.

Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---

Changelog v3:

* newly added this patch, to move skb_copy()->skb_clone() changes from
  PATCH 01/03 to a separate patch with its own explanation

 net/batman-adv/send.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)
  

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
  

On Monday, 17 May 2021 00:33:09 CEST Linus Lüssing wrote:
> +       /* __batadv_forw_bcast_packet clones, make sure original
> +        * skb stays writeable
> +        */
> +       return (skb_cow(skb, 0) < 0) ? NETDEV_TX_BUSY : NETDEV_TX_OK;

Just because we had this discussion a couple of hours ago: My last comment 
from May was that the skb_cow might be unnecessary - not that you need a 
skb_copy.

You wrote (for situation B):

> 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.

And my reply to this was:

> 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