| Message ID | 20210812145241.12449-1-joamaki@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 39a0876d595bd7c7512782dfcce0ee66f65bf221 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net, bonding: Disallow vlan+srcmac with XDP | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: daniel@iogearbox.net; 6 maintainers not CCed: davem@davemloft.net vfalico@gmail.com kuba@kernel.org daniel@iogearbox.net j.vosburgh@gmail.com andy@greyhouse.net |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3 this patch: 3 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 84 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3 this patch: 3 |
| netdev/header_inline | success | Link |
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
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.
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
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; > } >
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
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; }
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(-)