On 03/18/2018 01:12 PM, Sven Eckelmann wrote:
batadv_check_unicast_ttvn may redirect a packet to itself or another originator. This involves rewriting the ttvn and the destination address in the batadv unicast header. These field were not yet pulled (with skb rcsum update) and thus any change to them also requires a change in the receive checksum.
Reported-by: Matthias Schiffer mschiffer@universe-factory.net Fixes: cea194d90b11 ("batman-adv: improved client announcement mechanism") Signed-off-by: Sven Eckelmann sven@narfation.org
Cc: Matthias Schiffer mschiffer@universe-factory.net
This is an alternative fix for the problem reported by Matthias and his patch https://patchwork.open-mesh.org/patch/17305/. I've prepared it because I don't feel good about fixing one thing and at the same time add regression to another thing (even when it is only the debug output for arp packets) - at least not when this patch should end up at stable@vger.kernel.org
This patch was not actually tested. But it should help to see a different approach. Other code in the kernel which does something similar is:
- seg6_do_srh_inline
- set_eth_addr (openvswitch)
- set_nsh (openvswitch)
I would still like to merge the cleanup patches from Matthias - just not for net.git/stable@vger.kernel.org. Maybe Simon can decide about the maint patch - at least he will be the one who forwards them to DaveM.
Btw. in theory, only ttl+ttvn+dest have to be read to update the checksum correctly - but this patch also reads packet_type+version to keep the change simple.
[...]
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c index 0f10c565..cc3ed93a 100644 --- a/net/batman-adv/routing.c +++ b/net/batman-adv/routing.c @@ -759,6 +759,7 @@ static int batadv_route_unicast_packet(struct sk_buff *skb, /**
- batadv_reroute_unicast_packet() - update the unicast header for re-routing
- @bat_priv: the bat priv with all the soft interface information
- @skb: unicast packet to process
- @unicast_packet: the unicast header to be updated
- @dst_addr: the payload destination
- @vid: VLAN identifier
@@ -770,7 +771,7 @@ static int batadv_route_unicast_packet(struct sk_buff *skb,
- Return: true if the packet header has been updated, false otherwise
*/ static bool -batadv_reroute_unicast_packet(struct batadv_priv *bat_priv, +batadv_reroute_unicast_packet(struct batadv_priv *bat_priv, struct sk_buff *skb, struct batadv_unicast_packet *unicast_packet, u8 *dst_addr, unsigned short vid) { @@ -799,8 +800,10 @@ batadv_reroute_unicast_packet(struct batadv_priv *bat_priv, }
/* update the packet header */
- skb_postpull_rcsum(skb, unicast_packet, sizeof(*unicast_packet));
Using skb_postpull_rcsum here is incorrect: As the name indicates, it is supposed to be used after pulling, and will thus subtract the checksum of the header *before* skb->data, while we are interested in the sizeof(*unicast_packet) bytes *after* skb->data.
ether_addr_copy(unicast_packet->dest, orig_addr); unicast_packet->ttvn = orig_ttvn;
skb_postpush_rcsum(skb, unicast_packet, sizeof(*unicast_packet));
ret = true;
out: @@ -841,7 +844,7 @@ static bool batadv_check_unicast_ttvn(struct batadv_priv *bat_priv, * the packet to */ if (batadv_tt_local_client_is_roaming(bat_priv, ethhdr->h_dest, vid)) {
if (batadv_reroute_unicast_packet(bat_priv, unicast_packet,
if (batadv_reroute_unicast_packet(bat_priv, skb, unicast_packet, ethhdr->h_dest, vid)) batadv_dbg_ratelimited(BATADV_DBG_TT, bat_priv,@@ -887,7 +890,7 @@ static bool batadv_check_unicast_ttvn(struct batadv_priv *bat_priv, * destination can possibly be updated and forwarded towards the new * target host */
- if (batadv_reroute_unicast_packet(bat_priv, unicast_packet,
- if (batadv_reroute_unicast_packet(bat_priv, skb, unicast_packet, ethhdr->h_dest, vid)) { batadv_dbg_ratelimited(BATADV_DBG_TT, bat_priv, "Rerouting unicast packet to %pM (dst=%pM): TTVN mismatch old_ttvn=%u new_ttvn=%u\n",
@@ -910,12 +913,14 @@ static bool batadv_check_unicast_ttvn(struct batadv_priv *bat_priv, if (!primary_if) return false;
- /* update the packet header */
- skb_postpull_rcsum(skb, unicast_packet, sizeof(*unicast_packet));
Dito.
ether_addr_copy(unicast_packet->dest, primary_if->net_dev->dev_addr);
unicast_packet->ttvn = curr_ttvn;
skb_postpush_rcsum(skb, unicast_packet, sizeof(*unicast_packet));
batadv_hardif_put(primary_if);
- unicast_packet->ttvn = curr_ttvn;
- return true;
}
Regards, Matthias