Re: [PATCH v2] batman-adv: bcast: queue per interface, if needed

2021-05-14 Thread Sven Eckelmann
On Friday, 14 May 2021 13:35:32 CEST Linus Lüssing wrote:
> 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:
[...]

Haven't checked this in detail. But please split this part in a separate patch 
with the corresponding explanation. Should make it easier to understand why 
the skb_copy is no longer needed.

Kind regards,
Sven


signature.asc
Description: This is a digitally signed message part.


Re: [PATCH v2] batman-adv: bcast: queue per interface, if needed

2021-05-14 Thread Linus Lüssing
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


Re: [PATCH v2] batman-adv: bcast: queue per interface, if needed

2021-05-14 Thread Sven Eckelmann
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.

The review was stopped after noticing this problem.

Kind regards,
Sven

signature.asc
Description: This is a digitally signed message part.


[PATCH v2] batman-adv: bcast: queue per interface, if needed

2021-04-27 Thread Linus Lüssing
Currently we schedule a broadcast packet like:

3x: [ [(re-)queue] --> for(hard-if): maybe-transmit ]

The intention of queueing a broadcast packet multiple times is to
increase robustness for wireless interfaces. However on interfaces
which we only broadcast on once the queueing induces an unnecessary
penalty. This patch restructures the queueing to be performed on a per
interface basis:

for(hard-if):
- transmit
- if wireless: [queue] --> transmit --> [requeue] --> transmit

Next to the performance benefits on non-wireless interfaces this
should also make it easier to apply alternative strategies for
transmissions on wireless interfaces in the future (for instance sending
via unicast transmissions on wireless interfaces, without queueing in
batman-adv, if appropriate).

Signed-off-by: Linus Lüssing 
---

Changelog v2:
* removed the other two patches from the patchset for now, only
  the broadcast queueing cleanup to start with

* fixed spelling of "unnecessary" in commit message (thanks Sven)
* removed now superflous kerneldoc for hard_iface in
  batadv_forw_packet_bcasts_left() (thanks Sven)
* removed delay check for queued (re)broadcasts in
  batadv_forw_bcast_packet_if(): the only case where a delay is set
  for this function is for a delayed, DAT fallback ARP Request from
  this node, then however num_bcasts will be >=1, too, and the fallback
  ARP Request will be scheduled anyway
---
 net/batman-adv/main.h   |   1 -
 net/batman-adv/routing.c|   9 +-
 net/batman-adv/send.c   | 392 ++--
 net/batman-adv/send.h   |  12 +-
 net/batman-adv/soft-interface.c |  12 +-
 5 files changed, 288 insertions(+), 138 deletions(-)

diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h
index 8f0102b7..baa9fcbe 100644
--- a/net/batman-adv/main.h
+++ b/net/batman-adv/main.h
@@ -88,7 +88,6 @@
 /* number of packets to send for broadcasts on different interface types */
 #define BATADV_NUM_BCASTS_DEFAULT 1
 #define BATADV_NUM_BCASTS_WIRELESS 3
-#define BATADV_NUM_BCASTS_MAX 3
 
 /* length of the single packet used by the TP meter */
 #define BATADV_TP_PACKET_LEN ETH_DATA_LEN
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 40f5cffd..bb9e93e3 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -1182,9 +1182,9 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
struct batadv_bcast_packet *bcast_packet;
struct ethhdr *ethhdr;
int hdr_size = sizeof(*bcast_packet);
-   int ret = NET_RX_DROP;
s32 seq_diff;
u32 seqno;
+   int ret;
 
/* drop packet if it has not necessary minimum size */
if (unlikely(!pskb_may_pull(skb, hdr_size)))
@@ -1210,7 +1210,7 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
if (batadv_is_my_mac(bat_priv, bcast_packet->orig))
goto free_skb;
 
-   if (bcast_packet->ttl < 2)
+   if (bcast_packet->ttl-- < 2)
goto free_skb;
 
orig_node = batadv_orig_hash_find(bat_priv, bcast_packet->orig);
@@ -1249,7 +1249,9 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
batadv_skb_set_priority(skb, sizeof(struct batadv_bcast_packet));
 
/* rebroadcast packet */
-   batadv_add_bcast_packet_to_list(bat_priv, skb, 1, false);
+   ret = batadv_forw_bcast_packet(bat_priv, skb, 0, false);
+   if (ret == NETDEV_TX_BUSY)
+   goto free_skb;
 
/* don't hand the broadcast up if it is from an originator
 * from the same backbone.
@@ -1275,6 +1277,7 @@ int batadv_recv_bcast_packet(struct sk_buff *skb,
spin_unlock_bh(_node->bcast_seqno_lock);
 free_skb:
kfree_skb(skb);
+   ret = NET_RX_DROP;
 out:
if (orig_node)
batadv_orig_node_put(orig_node);
diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c
index 157abe92..1db6b217 100644
--- a/net/batman-adv/send.c
+++ b/net/batman-adv/send.c
@@ -737,57 +737,52 @@ void batadv_forw_packet_ogmv1_queue(struct batadv_priv 
*bat_priv,
 }
 
 /**
- * batadv_add_bcast_packet_to_list() - queue broadcast packet for multiple 
sends
+ * batadv_forw_bcast_packet_to_list() - queue broadcast packet for 
transmissions
  * @bat_priv: the bat priv with all the soft interface information
  * @skb: broadcast packet to add
  * @delay: number of jiffies to wait before sending
  * @own_packet: true if it is a self-generated broadcast packet
+ * @if_in: the interface where the packet was received on
+ * @if_out: the outgoing interface to queue on
  *
- * add a broadcast packet to the queue and setup timers. broadcast packets
+ * Adds a broadcast packet to the queue and sets up timers. Broadcast packets
  * are sent multiple times to increase probability for being received.
  *
- * 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