On Fri, May 14, 2021 at 10:28:53AM +0200, Sven Eckelmann wrote:
On Tuesday, 27 April 2021 20:45:27 CEST Linus Lüssing wrote:
- The skb is not consumed, so the caller should make sure that the
- skb is freed.
- This call clones the given skb, hence the caller needs to take into
- account that the data segment of the original skb might not be
- modifiable anymore.
But none of your callers is now taking care of it because you've removed all skb_copy's. All you do is to clone the control data and give it to the underlying layers. And they may write freely to the data. Thus breaking parallel (and under some circumstances sequential) running code which operates on the skbs.
Hi Sven,
Thanks for looking at it so far. I'm not quite sure if the skb_copy() is needed though. Because there is a new skb_cow(). Let me explain my thoughts:
We have two cases: A) Packet originating from ourself, via batadv_interface_tx(). Or B) received+forwarded from a neighbor node via batadv_recv_bcast_packet().
For case A), self generated:
When we send the packet multiple times, for each rebroadcast or interface we will push the ethernet header and write the ether-src, ether-dest and ether protocol in batadv_send_skb_packet(). Before that batadv_send_skb_packet() calls batadv_skb_head_push() which calls skb_cow_head(). So the ethernet header should be modifiable safely then, even if it is an skb clone.
For case B), received/forwarded:
Rebroadcasts same as in A), but additionally after rebroadcasting with potential requeuing in batadv_recv_bcast_packet() via batadv_forw_bcast_packet() we will also call batadv_interface_rx() and strip the batman header. Betweeen these calls there is the following though:
batadv_forw_bcast_packet(skb, ...) -> __batadv_forw_bcast_packet(skb, ...); ... skb_cow(skb, 0)
So the original skb will have been made uncloned/writeable again via the skb_cow() before being handed to batadv_interface_rx().
Let me know if I'm missing something.
Regards, Linus