| Message ID | e4ca1ef1-56f3-5bce-eec8-617e24bc7b1a@virtuozzo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [NET,v3,1/7] skbuff: introduce skb_expand_head() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | warning | Series does not have a cover letter |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| 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: 2 this patch: 2 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 46 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/header_inline | success | Link |
On 02.08.21 11:52, Vasily Averin wrote: > Unlike skb_realloc_headroom, new helper skb_expand_head > does not allocate a new skb if possible. > > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > --- > drivers/net/vrf.c | 21 +++++++-------------- > 1 file changed, 7 insertions(+), 14 deletions(-) > [...] > /* Be paranoid, rather than too clever. */ > if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) { > - struct sk_buff *skb2; > - > - skb2 = skb_realloc_headroom(skb, LL_RESERVED_SPACE(dev)); > - if (!skb2) { > - ret = -ENOMEM; > - goto err; > + skb = skb_expand_head(skb, hh_len); > + if (!skb) { > + skb->dev->stats.tx_errors++; > + return -ENOMEM; Hello Vasily, FYI, Coverity complains that we check skb != NULL here but then still dereference skb->dev: *** CID 1506214: Null pointer dereferences (FORWARD_NULL) /drivers/net/vrf.c: 867 in vrf_finish_output() 861 nf_reset_ct(skb); 862 863 /* Be paranoid, rather than too clever. */ 864 if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) { 865 skb = skb_expand_head(skb, hh_len); 866 if (!skb) { >>> CID 1506214: Null pointer dereferences (FORWARD_NULL) >>> Dereferencing null pointer "skb". 867 skb->dev->stats.tx_errors++; 868 return -ENOMEM; 869 } 870 } 871 872 rcu_read_lock_bh();
On 8/5/21 2:55 PM, Julian Wiedmann wrote: > On 02.08.21 11:52, Vasily Averin wrote: >> Unlike skb_realloc_headroom, new helper skb_expand_head >> does not allocate a new skb if possible. >> >> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> >> --- >> drivers/net/vrf.c | 21 +++++++-------------- >> 1 file changed, 7 insertions(+), 14 deletions(-) >> > > [...] > >> /* Be paranoid, rather than too clever. */ >> if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) { >> - struct sk_buff *skb2; >> - >> - skb2 = skb_realloc_headroom(skb, LL_RESERVED_SPACE(dev)); >> - if (!skb2) { >> - ret = -ENOMEM; >> - goto err; >> + skb = skb_expand_head(skb, hh_len); >> + if (!skb) { >> + skb->dev->stats.tx_errors++; >> + return -ENOMEM; > > Hello Vasily, > > FYI, Coverity complains that we check skb != NULL here but then > still dereference skb->dev: > > > *** CID 1506214: Null pointer dereferences (FORWARD_NULL) > /drivers/net/vrf.c: 867 in vrf_finish_output() > 861 nf_reset_ct(skb); > 862 > 863 /* Be paranoid, rather than too clever. */ > 864 if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) { > 865 skb = skb_expand_head(skb, hh_len); > 866 if (!skb) { >>>> CID 1506214: Null pointer dereferences (FORWARD_NULL) >>>> Dereferencing null pointer "skb". > 867 skb->dev->stats.tx_errors++; > 868 return -ENOMEM; My fault, I missed it. Thank you, Vasily Averin
currently if skb does not have enough headroom skb_realloc_headrom is called.
It is not optimal because it creates new skb.
this patch set introduces new helper skb_expand_head()
Unlike skb_realloc_headroom, it does not allocate a new skb if possible;
copies skb->sk on new skb when as needed and frees original skb in case of failures.
This helps to simplify ip[6]_finish_output2(), ip6_xmit() and few other
functions in vrf, ax25 and bpf.
There are few other cases where this helper can be used
but it requires an additional investigations.
v4 changes:
- fixed null pointer dereference in vrf patch reported by Julian Wiedmann
v3 changes:
- ax25 compilation warning fixed
- v5.14-rc4 rebase
- now it does not depend on non-committed pathces
v2 changes:
- helper's name was changed to skb_expand_head
- fixed few mistakes inside skb_expand_head():
skb_set_owner_w should set sk on nskb
kfree was replaced by kfree_skb()
improved warning message
- added minor refactoring in changed functions in vrf and bpf patches
- removed kfree_skb() in ax25_rt_build_path caller ax25_ip_xmit
Vasily Averin (7):
skbuff: introduce skb_expand_head()
ipv6: use skb_expand_head in ip6_finish_output2
ipv6: use skb_expand_head in ip6_xmit
ipv4: use skb_expand_head in ip_finish_output2
vrf: use skb_expand_head in vrf_finish_output
ax25: use skb_expand_head
bpf: use skb_expand_head in bpf_out_neigh_v4/6
drivers/net/vrf.c | 23 ++++++---------
include/linux/skbuff.h | 1 +
net/ax25/ax25_ip.c | 4 +--
net/ax25/ax25_out.c | 13 ++-------
net/ax25/ax25_route.c | 13 ++-------
net/core/filter.c | 27 ++++-------------
net/core/skbuff.c | 42 +++++++++++++++++++++++++++
net/ipv4/ip_output.c | 13 ++-------
net/ipv6/ip6_output.c | 78 +++++++++++++++++---------------------------------
9 files changed, 92 insertions(+), 122 deletions(-)
diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c index 2b1b944..726adf0 100644 --- a/drivers/net/vrf.c +++ b/drivers/net/vrf.c @@ -857,30 +857,24 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s unsigned int hh_len = LL_RESERVED_SPACE(dev); struct neighbour *neigh; bool is_v6gw = false; - int ret = -EINVAL; nf_reset_ct(skb); /* Be paranoid, rather than too clever. */ if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) { - struct sk_buff *skb2; - - skb2 = skb_realloc_headroom(skb, LL_RESERVED_SPACE(dev)); - if (!skb2) { - ret = -ENOMEM; - goto err; + skb = skb_expand_head(skb, hh_len); + if (!skb) { + skb->dev->stats.tx_errors++; + return -ENOMEM; } - if (skb->sk) - skb_set_owner_w(skb2, skb->sk); - - consume_skb(skb); - skb = skb2; } rcu_read_lock_bh(); neigh = ip_neigh_for_gw(rt, skb, &is_v6gw); if (!IS_ERR(neigh)) { + int ret; + sock_confirm_neigh(skb, neigh); /* if crossing protocols, can not use the cached header */ ret = neigh_output(neigh, skb, is_v6gw); @@ -889,9 +883,8 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s } rcu_read_unlock_bh(); -err: vrf_tx_error(skb->dev, skb); - return ret; + return -EINVAL; } static int vrf_output(struct net *net, struct sock *sk, struct sk_buff *skb)
Unlike skb_realloc_headroom, new helper skb_expand_head does not allocate a new skb if possible. Signed-off-by: Vasily Averin <vvs@virtuozzo.com> --- drivers/net/vrf.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)