[PATCH net-next] net, bonding: Disallow vlan+srcmac with XDP

[PATCH net-next] net, bonding: Disallow vlan+srcmac with XDP

[Jussi Maki]

The new vlan+srcmac xmit policy is not implementable with XDP since
in many cases the 802.1Q payload is not present in the packet. This
can be for example due to hardware offload or in the case of veth
due to use of skbuffs internally.

This also fixes the NULL deref with the vlan+srcmac xmit policy
reported by Jonathan Toppins by additionally checking the skb
pointer.

Fixes: a815bde56b15 ("net, bonding: Refactor bond_xmit_hash for use with xdp_buff")
Reported-by: Jonathan Toppins <jtoppins@redhat.com>
Signed-off-by: Jussi Maki <joamaki@gmail.com>
---
 drivers/net/bonding/bond_main.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c0db4e2b2462..04158a8368e4 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -322,9 +322,15 @@ static bool bond_xdp_check(struct bonding *bond)
 	switch (BOND_MODE(bond)) {
 	case BOND_MODE_ROUNDROBIN:
 	case BOND_MODE_ACTIVEBACKUP:
+		return true;
 	case BOND_MODE_8023AD:
 	case BOND_MODE_XOR:
-		return true;
+		/* vlan+srcmac is not supported with XDP as in most cases the 802.1q
+		 * payload is not in the packet due to hardware offload.
+		 */
+		if (bond->params.xmit_policy != BOND_XMIT_POLICY_VLAN_SRCMAC)
+			return true;
+		fallthrough;
 	default:
 		return false;
 	}
@@ -3744,9 +3750,9 @@ static bool bond_flow_ip(struct sk_buff *skb, struct flow_keys *fk, const void *
 
 static u32 bond_vlan_srcmac_hash(struct sk_buff *skb, const void *data, int mhoff, int hlen)
 {
-	struct ethhdr *mac_hdr;
 	u32 srcmac_vendor = 0, srcmac_dev = 0;
-	u16 vlan;
+	struct ethhdr *mac_hdr;
+	u16 vlan = 0;
 	int i;
 
 	data = bond_pull_data(skb, data, hlen, mhoff + sizeof(struct ethhdr));
@@ -3760,10 +3766,8 @@ static u32 bond_vlan_srcmac_hash(struct sk_buff *skb, const void *data, int mhof
 	for (i = 3; i < ETH_ALEN; i++)
 		srcmac_dev = (srcmac_dev << 8) | mac_hdr->h_source[i];
 
-	if (!skb_vlan_tag_present(skb))
-		return srcmac_vendor ^ srcmac_dev;
-
-	vlan = skb_vlan_tag_get(skb);
+	if (skb && skb_vlan_tag_present(skb))
+		vlan = skb_vlan_tag_get(skb);
 
 	return vlan ^ srcmac_vendor ^ srcmac_dev;
 }
-- 
2.17.1

Re: [PATCH net-next] net, bonding: Disallow vlan+srcmac with XDP

[Nikolay Aleksandrov]

On 12/08/2021 17:52, Jussi Maki wrote:
> The new vlan+srcmac xmit policy is not implementable with XDP since
> in many cases the 802.1Q payload is not present in the packet. This
> can be for example due to hardware offload or in the case of veth
> due to use of skbuffs internally.
> 
> This also fixes the NULL deref with the vlan+srcmac xmit policy
> reported by Jonathan Toppins by additionally checking the skb
> pointer.
> 
> Fixes: a815bde56b15 ("net, bonding: Refactor bond_xmit_hash for use with xdp_buff")
> Reported-by: Jonathan Toppins <jtoppins@redhat.com>
> Signed-off-by: Jussi Maki <joamaki@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 

Hi Jussi,
Could you please share the null ptr deref trace?
I'm curious how we can get a null skb at that point.

Also how are the xdp and null ptr deref changes related ?

Thanks,
 Nik

Re: [PATCH net-next] net, bonding: Disallow vlan+srcmac with XDP

[Jussi Maki]

On Thu, Aug 12, 2021 at 5:01 PM Nikolay Aleksandrov <nikolay@nvidia.com> wrote:
> Hi Jussi,
> Could you please share the null ptr deref trace?
> I'm curious how we can get a null skb at that point.

Hi Nik, this was reported by Jonathan here:
https://lore.kernel.org/bpf/20210728234350.28796-1-joamaki@gmail.com/T/#m07a73b1886a9213feb7112ce2a0d6dfde84fd27a.
I didn't reproduce the null ptr deref as it was fairly obvious how it
can happen, e.g. by having a bond with xmit_policy=vlan+srcmac. The
hashing functions were refactored to be used for both xdp_buff and
skbuff uses and the skb pointer became optional (was meant to be used
when packet was non-linear), but I missed fixing the vlan hashing
function. Partially the reason leading to this was that the
xmit_policy is very new and the bpf vmtest infra still uses an older
iproute2 version which didn't support it, so this was untested. What
is not tested is broken as usual.

> Also how are the xdp and null ptr deref changes related ?

They're related in that looking into the null ptr deref here I
realized that vlan+srcmac didn't make sense with XDP since we have no
guarantee that the vlan id is in the ethernet header. So this patch
both fixes the deref by checking the skb pointer for NULL and it
disallows the whole xmit policy for XDP for the aforementioned reason.

Hope this makes sense.

Re: [PATCH net-next] net, bonding: Disallow vlan+srcmac with XDP

[Nikolay Aleksandrov]

On 12/08/2021 18:12, Jussi Maki wrote:
> On Thu, Aug 12, 2021 at 5:01 PM Nikolay Aleksandrov <nikolay@nvidia.com> wrote:
>> Hi Jussi,
>> Could you please share the null ptr deref trace?
>> I'm curious how we can get a null skb at that point.
> 
> Hi Nik, this was reported by Jonathan here:
> https://lore.kernel.org/bpf/20210728234350.28796-1-joamaki@gmail.com/T/#m07a73b1886a9213feb7112ce2a0d6dfde84fd27a.
> I didn't reproduce the null ptr deref as it was fairly obvious how it
> can happen, e.g. by having a bond with xmit_policy=vlan+srcmac. The
> hashing functions were refactored to be used for both xdp_buff and
> skbuff uses and the skb pointer became optional (was meant to be used
> when packet was non-linear), but I missed fixing the vlan hashing
> function. Partially the reason leading to this was that the
> xmit_policy is very new and the bpf vmtest infra still uses an older
> iproute2 version which didn't support it, so this was untested. What
> is not tested is broken as usual.
> 
>> Also how are the xdp and null ptr deref changes related ?
> 
> They're related in that looking into the null ptr deref here I
> realized that vlan+srcmac didn't make sense with XDP since we have no
> guarantee that the vlan id is in the ethernet header. So this patch
> both fixes the deref by checking the skb pointer for NULL and it
> disallows the whole xmit policy for XDP for the aforementioned reason.
> 
> Hope this makes sense.
> 

Oh, I had totally missed the bond xdp patch-set, all makes sense now.

Thanks,
 Nik

Re: [PATCH net-next] net, bonding: Disallow vlan+srcmac with XDP

[Jonathan Toppins]

On 8/12/21 10:52 AM, Jussi Maki wrote:
> The new vlan+srcmac xmit policy is not implementable with XDP since
> in many cases the 802.1Q payload is not present in the packet. This
> can be for example due to hardware offload or in the case of veth
> due to use of skbuffs internally.
> 
> This also fixes the NULL deref with the vlan+srcmac xmit policy
> reported by Jonathan Toppins by additionally checking the skb
> pointer.
> 
> Fixes: a815bde56b15 ("net, bonding: Refactor bond_xmit_hash for use with xdp_buff")
> Reported-by: Jonathan Toppins <jtoppins@redhat.com>
> Signed-off-by: Jussi Maki <joamaki@gmail.com>

Looks good, thanks.

Reviewed-by: Jonathan Toppins <jtoppins@redhat.com>

> ---
>   drivers/net/bonding/bond_main.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index c0db4e2b2462..04158a8368e4 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -322,9 +322,15 @@ static bool bond_xdp_check(struct bonding *bond)
>   	switch (BOND_MODE(bond)) {
>   	case BOND_MODE_ROUNDROBIN:
>   	case BOND_MODE_ACTIVEBACKUP:
> +		return true;
>   	case BOND_MODE_8023AD:
>   	case BOND_MODE_XOR:
> -		return true;
> +		/* vlan+srcmac is not supported with XDP as in most cases the 802.1q
> +		 * payload is not in the packet due to hardware offload.
> +		 */
> +		if (bond->params.xmit_policy != BOND_XMIT_POLICY_VLAN_SRCMAC)
> +			return true;
> +		fallthrough;
>   	default:
>   		return false;
>   	}
> @@ -3744,9 +3750,9 @@ static bool bond_flow_ip(struct sk_buff *skb, struct flow_keys *fk, const void *
>   
>   static u32 bond_vlan_srcmac_hash(struct sk_buff *skb, const void *data, int mhoff, int hlen)
>   {
> -	struct ethhdr *mac_hdr;
>   	u32 srcmac_vendor = 0, srcmac_dev = 0;
> -	u16 vlan;
> +	struct ethhdr *mac_hdr;
> +	u16 vlan = 0;
>   	int i;
>   
>   	data = bond_pull_data(skb, data, hlen, mhoff + sizeof(struct ethhdr));
> @@ -3760,10 +3766,8 @@ static u32 bond_vlan_srcmac_hash(struct sk_buff *skb, const void *data, int mhof
>   	for (i = 3; i < ETH_ALEN; i++)
>   		srcmac_dev = (srcmac_dev << 8) | mac_hdr->h_source[i];
>   
> -	if (!skb_vlan_tag_present(skb))
> -		return srcmac_vendor ^ srcmac_dev;
> -
> -	vlan = skb_vlan_tag_get(skb);
> +	if (skb && skb_vlan_tag_present(skb))
> +		vlan = skb_vlan_tag_get(skb);
>   
>   	return vlan ^ srcmac_vendor ^ srcmac_dev;
>   }
> 

Re: [PATCH net-next] net, bonding: Disallow vlan+srcmac with XDP

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Thu, 12 Aug 2021 14:52:41 +0000 you wrote:
> The new vlan+srcmac xmit policy is not implementable with XDP since
> in many cases the 802.1Q payload is not present in the packet. This
> can be for example due to hardware offload or in the case of veth
> due to use of skbuffs internally.
> 
> This also fixes the NULL deref with the vlan+srcmac xmit policy
> reported by Jonathan Toppins by additionally checking the skb
> pointer.
> 
> [...]

Here is the summary with links:
  - [net-next] net, bonding: Disallow vlan+srcmac with XDP
    https://git.kernel.org/netdev/net-next/c/39a0876d595b

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html