diff mbox series

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

Message ID 20210516223309.12596-3-linus.luessing@c0d3.blue
State Superseded, archived
Delegated to: Sven Eckelmann
Headers show
Series [v3,1/3] batman-adv: bcast: queue per interface, if needed | expand

Commit Message

Linus Lüssing May 16, 2021, 10:33 p.m. UTC
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(-)

Comments

Sven Eckelmann May 30, 2021, 11:52 a.m. UTC | #1
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
Sven Eckelmann Aug. 17, 2021, 12:24 p.m. UTC | #2
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
diff mbox series

Patch

diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c
index 0b9dd29d..1db6b217 100644
--- a/net/batman-adv/send.c
+++ b/net/batman-adv/send.c
@@ -748,6 +748,10 @@  void batadv_forw_packet_ogmv1_queue(struct batadv_priv *bat_priv,
  * Adds a broadcast packet to the queue and sets up timers. Broadcast packets
  * are sent multiple times to increase probability for being received.
  *
+ * 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.
+ *
  * Return: NETDEV_TX_OK on success and NETDEV_TX_BUSY on errors.
  */
 static int batadv_forw_bcast_packet_to_list(struct batadv_priv *bat_priv,
@@ -761,7 +765,7 @@  static int batadv_forw_bcast_packet_to_list(struct batadv_priv *bat_priv,
 	unsigned long send_time = jiffies;
 	struct sk_buff *newskb;
 
-	newskb = skb_copy(skb, GFP_ATOMIC);
+	newskb = skb_clone(skb, GFP_ATOMIC);
 	if (!newskb)
 		goto err;
 
@@ -800,6 +804,10 @@  static int batadv_forw_bcast_packet_to_list(struct batadv_priv *bat_priv,
  * or if a delay is given after that. Furthermore, queues additional
  * retransmissions if this interface is a wireless one.
  *
+ * 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.
+ *
  * Return: NETDEV_TX_OK on success and NETDEV_TX_BUSY on errors.
  */
 static int batadv_forw_bcast_packet_if(struct batadv_priv *bat_priv,
@@ -814,7 +822,7 @@  static int batadv_forw_bcast_packet_if(struct batadv_priv *bat_priv,
 	int ret = NETDEV_TX_OK;
 
 	if (!delay) {
-		newskb = skb_copy(skb, GFP_ATOMIC);
+		newskb = skb_clone(skb, GFP_ATOMIC);
 		if (!newskb)
 			return NETDEV_TX_BUSY;
 
@@ -966,6 +974,8 @@  static int __batadv_forw_bcast_packet(struct batadv_priv *bat_priv,
  * after that. Furthermore, queues additional retransmissions on wireless
  * interfaces.
  *
+ * This call might reallocate skb data.
+ *
  * Return: NETDEV_TX_OK on success and NETDEV_TX_BUSY on errors.
  */
 int batadv_forw_bcast_packet(struct batadv_priv *bat_priv,
@@ -973,7 +983,15 @@  int batadv_forw_bcast_packet(struct batadv_priv *bat_priv,
 			     unsigned long delay,
 			     bool own_packet)
 {
-	return __batadv_forw_bcast_packet(bat_priv, skb, delay, own_packet);
+	int ret = __batadv_forw_bcast_packet(bat_priv, skb, delay, own_packet);
+
+	if (ret == NETDEV_TX_BUSY)
+		return ret;
+
+	/* __batadv_forw_bcast_packet clones, make sure original
+	 * skb stays writeable
+	 */
+	return (skb_cow(skb, 0) < 0) ? NETDEV_TX_BUSY : NETDEV_TX_OK;
 }
 
 /**