Re: [PATCH net] bonding: do not assume skb mac_header is set

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Eric Dumazet <edumazet@google.com> wrote:

>On Thu, Jun 22, 2023 at 7:48 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>>
>> Eric Dumazet <edumazet@google.com> wrote:

[...]

>> > drivers/net/bonding/bond_main.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >index edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
>> >--- a/drivers/net/bonding/bond_main.c
>> >+++ b/drivers/net/bonding/bond_main.c
>> >@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>> >               return skb->hash;
>> >
>> >       return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
>> >-                              skb_mac_offset(skb), skb_network_offset(skb),
>> >+                              0, skb_network_offset(skb),
>> >                               skb_headlen(skb));
>> > }
>>
>>         Is the MAC header guaranteed to be at skb->data, then?  If not,
>> then isn't replacing skb_mac_offset() with 0 going to break the hash (as
>> it might or might not be looking at the actual MAC header)?
>>
>
>In ndo_start_xmit(), skb->data points to MAC header by definition.

	Ok.

>>         Also, assuming for the moment that this change is ok, this makes
>> all callers of __bond_xmit_hash() supply zero for the mhoff parameter,
>> and a complete fix should therefore remove the unused parameter and its
>> various references.
>
>Not really: bond_xmit_hash_xdp() calls __bond_xmit_hash() with
>sizeof(struct ethhdr)

	I don't think so:

static u32 __bond_xmit_hash(struct bonding *bond, struct sk_buff *skb, const void *data,
                            __be16 l2_proto, int mhoff, int nhoff, int hlen)
{

	"mhoff", currently supplied as skb_mac_offset(skb) in
bond_xmit_hash(), is the fifth parameter.

static u32 bond_xmit_hash_xdp(struct bonding *bond, struct xdp_buff *xdp)
{
[...]
        return __bond_xmit_hash(bond, NULL, xdp->data, eth->h_proto, 0,
                                sizeof(struct ethhdr), xdp->data_end - xdp->data);
}

	The fifth argument here is 0.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com