[PATCH NET v3 5/7] vrf: use skb_expand_head in vrf_finish_output

Unlike skb_realloc_headroom, new helper skb_expand_head
does not allocate a new skb if possible.

Signed-off-by: Vasily Averin <redacted>
---
 drivers/net/vrf.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 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)

-- 
1.8.3.1

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 <redacted>
---
 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) {

quoted

quoted

    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:

quoted

Unlike skb_realloc_headroom, new helper skb_expand_head
does not allocate a new skb if possible.

Signed-off-by: Vasily Averin <redacted>
---
 drivers/net/vrf.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

[...]

quoted

 	/* 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) {

quoted

quoted

quoted

    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(-)

-- 
1.8.3.1

I already applied v3 to net-next, please send a relative fixup if you want to incorpoate the v4 changes too.

Thank you.

After 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output")
skb->dev  is accessed after skb free.
Let's replace skb->dev by dev = skb_dst(skb)->dev:
vrf_finish_output() is only called from vrf_output(),
it set skb->dev to skb_dst(skb)->dev and calls POSTROUTING netfilter
hooks, where output device should not be changed.

Fixes: 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output")
Reported-by: Julian Wiedmann <redacted>
Signed-off-by: Vasily Averin <redacted>
---
 drivers/net/vrf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 726adf0..168d4ef 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c

@@ -864,7 +864,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s
 	if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) {
 		skb = skb_expand_head(skb, hh_len);
 		if (!skb) {
-			skb->dev->stats.tx_errors++;
+			dev->stats.tx_errors++;
 			return -ENOMEM;
 		}
 	}

@@ -883,7 +883,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s
 	}
 
 	rcu_read_unlock_bh();
-	vrf_tx_error(skb->dev, skb);
+	vrf_tx_error(dev, skb);
 	return -EINVAL;
 }
 

-- 
1.8.3.1

On Fri, 6 Aug 2021 15:53:00 +0300 Vasily Averin wrote:

After 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output")
skb->dev  is accessed after skb free.
Let's replace skb->dev by dev = skb_dst(skb)->dev:
vrf_finish_output() is only called from vrf_output(),
it set skb->dev to skb_dst(skb)->dev and calls POSTROUTING netfilter
hooks, where output device should not be changed.

Fixes: 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output")
Reported-by: Julian Wiedmann <redacted>
Signed-off-by: Vasily Averin <redacted>

Thanks for following up! I decided to pick a similar patch from Dan
Carpenter [1] because the chunk quoted below is not really necessary.

[1] https://lore.kernel.org/kernel-janitors/20210806150435.GB15586@kili/

On 8/7/21 1:42 AM, Jakub Kicinski wrote:

On Fri, 6 Aug 2021 15:53:00 +0300 Vasily Averin wrote:

quoted

After 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output")
skb->dev  is accessed after skb free.
Let's replace skb->dev by dev = skb_dst(skb)->dev:
vrf_finish_output() is only called from vrf_output(),
it set skb->dev to skb_dst(skb)->dev and calls POSTROUTING netfilter
hooks, where output device should not be changed.

Fixes: 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output")
Reported-by: Julian Wiedmann <redacted>
Signed-off-by: Vasily Averin <redacted>

Thanks for following up! I decided to pick a similar patch from Dan
Carpenter [1] because the chunk quoted below is not really necessary.

I still think that my patch version is preferable.
It's better to use vrf_tx_error(dev, skb) because:
a) both rollbacks can use the same net device
b) probably using 'dev' allows to avoid an extra pointer dereference.

Originally, i.e. before fixed patch 14ee70ca89e6, rollback after failed header expand
called the save vrf_tx_error() call. This function does 2 things:  
- increments stats.tx_errors on specified network device
- frees provided skb.

Commit 14ee70ca89e6 replaced skb_realloc_headroom() by skb_expand_head() that frees skb inside,
So vrf_tx_error() call on rollback was replaced with direct increment of  stats.tx_errors.
We cannot use now original skb->dev so our fixup patches replaces it with dev variable already
used in this function.
Though, if we should use the same net device in both rollbacks. It's illogical for me
to change one place and do not change another one. 

If we follow to your decision -- it isn't a problem. skb->dev and skb should be identical.
Though 'skb->dev' does an extra dereference, while dev was used in function and probably
was saved to register.

Thank you,
	Vasily Averin

[1] https://lore.kernel.org/kernel-janitors/20210806150435.GB15586@kili/

quoted

@@ -883,7 +883,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s
 	}
 
 	rcu_read_unlock_bh();
-	vrf_tx_error(skb->dev, skb);
+	vrf_tx_error(dev, skb);
 	return -EINVAL;
 }