[PATCH v1 net-next 0/5] ipv6: Random cleanup for Extension Header.

[PATCH v1 net-next 0/5] ipv6: Random cleanup for Extension Header.

[Kuniyuki Iwashima]

This series (1) cleans up pskb_may_pull() in some functions, where needed
data are already pulled by their caller, (2) removes redundant multicast
test, and (3) optimises reload timing of the header.


Kuniyuki Iwashima (5):
  ipv6: rpl: Remove pskb(_may)?_pull() in ipv6_rpl_srh_rcv().
  ipv6: rpl: Remove redundant multicast tests in ipv6_rpl_srh_rcv().
  ipv6: exthdrs: Replace pskb_pull() with skb_pull() in ipv6_srh_rcv().
  ipv6: exthdrs: Reload hdr only when needed in ipv6_srh_rcv().
  ipv6: exthdrs: Remove redundant skb_headlen() check in
    ip6_parse_tlv().

 include/net/rpl.h  |  3 ---
 net/ipv6/exthdrs.c | 33 +++++----------------------------
 net/ipv6/rpl.c     |  7 -------
 3 files changed, 5 insertions(+), 38 deletions(-)

-- 
2.30.2

[PATCH v1 net-next 1/5] ipv6: rpl: Remove pskb(_may)?_pull() in ipv6_rpl_srh_rcv().

[Kuniyuki Iwashima]

As Eric Dumazet pointed out [0], ipv6_rthdr_rcv() pulls these data

  - Segment Routing Header : 8
  - Hdr Ext Len            : skb_transport_header(skb)[1] << 3

needed by ipv6_rpl_srh_rcv().  We can remove pskb_may_pull() and
replace pskb_pull() with skb_pull() in ipv6_rpl_srh_rcv().

Link: https://lore.kernel.org/netdev/CANn89iLboLwLrHXeHJucAqBkEL_S0rJFog68t7wwwXO-aNf5Mg@mail.gmail.com/ [0]
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/rpl.h  |  3 ---
 net/ipv6/exthdrs.c | 17 +----------------
 net/ipv6/rpl.c     |  7 -------
 3 files changed, 1 insertion(+), 26 deletions(-)

diff --git a/include/net/rpl.h b/include/net/rpl.h
index 30fe780d1e7c..74734191c458 100644
--- a/include/net/rpl.h
+++ b/include/net/rpl.h
@@ -23,9 +23,6 @@ static inline int rpl_init(void)
 static inline void rpl_exit(void) {}
 #endif
 
-size_t ipv6_rpl_srh_size(unsigned char n, unsigned char cmpri,
-			 unsigned char cmpre);
-
 void ipv6_rpl_srh_decompress(struct ipv6_rpl_sr_hdr *outhdr,
 			     const struct ipv6_rpl_sr_hdr *inhdr,
 			     const struct in6_addr *daddr, unsigned char n);
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index a543df57801f..65adc11b59aa 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -517,11 +517,7 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
 
 			skb_postpull_rcsum(skb, skb_network_header(skb),
 					   skb_network_header_len(skb));
-
-			if (!pskb_pull(skb, offset)) {
-				kfree_skb(skb);
-				return -1;
-			}
+			skb_pull(skb, offset);
 			skb_postpull_rcsum(skb, skb_transport_header(skb),
 					   offset);
 
@@ -543,11 +539,6 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
 		return 1;
 	}
 
-	if (!pskb_may_pull(skb, sizeof(*hdr))) {
-		kfree_skb(skb);
-		return -1;
-	}
-
 	n = (hdr->hdrlen << 3) - hdr->pad - (16 - hdr->cmpre);
 	r = do_div(n, (16 - hdr->cmpri));
 	/* checks if calculation was without remainder and n fits into
@@ -567,12 +558,6 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
 		return -1;
 	}
 
-	if (!pskb_may_pull(skb, ipv6_rpl_srh_size(n, hdr->cmpri,
-						  hdr->cmpre))) {
-		kfree_skb(skb);
-		return -1;
-	}
-
 	hdr->segments_left--;
 	i = n - hdr->segments_left;
 
diff --git a/net/ipv6/rpl.c b/net/ipv6/rpl.c
index d1876f192225..e186998bfbf7 100644
--- a/net/ipv6/rpl.c
+++ b/net/ipv6/rpl.c
@@ -29,13 +29,6 @@ static void *ipv6_rpl_segdata_pos(const struct ipv6_rpl_sr_hdr *hdr, int i)
 	return (void *)&hdr->rpl_segdata[i * IPV6_PFXTAIL_LEN(hdr->cmpri)];
 }
 
-size_t ipv6_rpl_srh_size(unsigned char n, unsigned char cmpri,
-			 unsigned char cmpre)
-{
-	return sizeof(struct ipv6_rpl_sr_hdr) + (n * IPV6_PFXTAIL_LEN(cmpri)) +
-		IPV6_PFXTAIL_LEN(cmpre);
-}
-
 void ipv6_rpl_srh_decompress(struct ipv6_rpl_sr_hdr *outhdr,
 			     const struct ipv6_rpl_sr_hdr *inhdr,
 			     const struct in6_addr *daddr, unsigned char n)
-- 
2.30.2

Re: [PATCH v1 net-next 1/5] ipv6: rpl: Remove pskb(_may)?_pull() in ipv6_rpl_srh_rcv().

[Jakub Kicinski]

On Wed, 14 Jun 2023 16:01:03 -0700 Kuniyuki Iwashima wrote:
> -	if (!pskb_may_pull(skb, ipv6_rpl_srh_size(n, hdr->cmpri,
> -						  hdr->cmpre))) {

Are we checking that 

	ipv6_rpl_srh_size(n, hdr->cmpri, hdr->cmpre) < (hdrlen + 1) << 3

somewhere?

also nit:

> As Eric Dumazet pointed out [0], ipv6_rthdr_rcv() pulls these data
> 
>   - Segment Routing Header : 8
>   - Hdr Ext Len            : skb_transport_header(skb)[1] << 3

+1 missing here, AFAICT

Re: [PATCH v1 net-next 1/5] ipv6: rpl: Remove pskb(_may)?_pull() in ipv6_rpl_srh_rcv().

[Kuniyuki Iwashima]

From: Jakub Kicinski <kuba@kernel.org>
Date: Sat, 17 Jun 2023 01:00:14 -0700
> On Wed, 14 Jun 2023 16:01:03 -0700 Kuniyuki Iwashima wrote:
> > -	if (!pskb_may_pull(skb, ipv6_rpl_srh_size(n, hdr->cmpri,
> > -						  hdr->cmpre))) {
> 
> Are we checking that 
> 
> 	ipv6_rpl_srh_size(n, hdr->cmpri, hdr->cmpre) < (hdrlen + 1) << 3
> 
> somewhere?

sizeof(struct ipv6_rpl_sr_hdr) is (1 << 3), and n is calculated from
hdrlen, cmpri, cmpre, and pad.

Here n could underflow, but the irregular case is caught by the two
conditionals below, where segments_left >= 1.

  1) n <  U64_MAX -> n + 1 > 255
  2) n == U64_MAX -> n + 1 == 0 < segments_left

So, the formula below holds.

  n1 = (hdr->hdrlen << 3) - hdr->pad - (16 - hdr->cmpre)
  n2 = n1 / (16 - hdr->cmpri)

  n2 * (16 - hdr->cmpri) + hdr->pad + (16 - hdr->cmpre) = hdr->hdrlen << 3
  (Here pad could be equal or greater than 0)

  n2 * (16 - hdr->cmpri) + (16 - hdr->cmpre) <= hdr->hdrlen << 3

  ipv6_rpl_srh_size(n, hdr->cmpri, hdr->cmpre) <= (hdrlen + 1) << 3

---8<---
	n = (hdr->hdrlen << 3) - hdr->pad - (16 - hdr->cmpre);
	r = do_div(n, (16 - hdr->cmpri));
	/* checks if calculation was without remainder and n fits into
	 * unsigned char which is segments_left field. Should not be
	 * higher than that.
	 */
	if (r || (n + 1) > 255) {
		kfree_skb(skb);
		return -1;
	}

	if (hdr->segments_left > n + 1) {
...
		return -1;
	}

	hdr->segments_left--;
	i = n - hdr->segments_left;
---8<---


> 
> also nit:
> 
> > As Eric Dumazet pointed out [0], ipv6_rthdr_rcv() pulls these data
> > 
> >   - Segment Routing Header : 8
> >   - Hdr Ext Len            : skb_transport_header(skb)[1] << 3
> 
> +1 missing here, AFAICT

The +1 here is the 8 bytes header.  Hdr Ext Len does not include
the header length.

In ipv6_rthdr_rcv(), the first pskb_may_pull() expresses it as 8,
and 2nd one uses (1 << 3).  We cannot use sizeof() here because
each Ext Hdr has different struct, although all of them are 8 bytes.

---8<---
	if (!pskb_may_pull(skb, skb_transport_offset(skb) + 8) ||
	    !pskb_may_pull(skb, (skb_transport_offset(skb) +
				 ((skb_transport_header(skb)[1] + 1) << 3)))) {
		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
		kfree_skb(skb);
		return -1;
	}
---8<---

Thanks!

[PATCH v1 net-next 2/5] ipv6: rpl: Remove redundant multicast tests in ipv6_rpl_srh_rcv().

[Kuniyuki Iwashima]

ipv6_rpl_srh_rcv() checks if ipv6_hdr(skb)->daddr or ohdr->rpl_segaddr[i]
is the multicast address with ipv6_addr_type().

We have the same check for ipv6_hdr(skb)->daddr in ipv6_rthdr_rcv(), so we
need not recheck it in ipv6_rpl_srh_rcv().

Also, we should use ipv6_addr_is_multicast() for ohdr->rpl_segaddr[i]
instead of ipv6_addr_type().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv6/exthdrs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 65adc11b59aa..6259e907f0d9 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -571,8 +571,7 @@ static int ipv6_rpl_srh_rcv(struct sk_buff *skb)
 	ipv6_rpl_srh_decompress(ohdr, hdr, &ipv6_hdr(skb)->daddr, n);
 	chdr = (struct ipv6_rpl_sr_hdr *)(buf + ((ohdr->hdrlen + 1) << 3));
 
-	if ((ipv6_addr_type(&ipv6_hdr(skb)->daddr) & IPV6_ADDR_MULTICAST) ||
-	    (ipv6_addr_type(&ohdr->rpl_segaddr[i]) & IPV6_ADDR_MULTICAST)) {
+	if (ipv6_addr_is_multicast(&ohdr->rpl_segaddr[i])) {
 		kfree_skb(skb);
 		kfree(buf);
 		return -1;
-- 
2.30.2

[PATCH v1 net-next 3/5] ipv6: exthdrs: Replace pskb_pull() with skb_pull() in ipv6_srh_rcv().

[Kuniyuki Iwashima]

ipv6_rthdr_rcv() pulls these data

  - Segment Routing Header : 8
  - Hdr Ext Len            : skb_transport_header(skb)[1] << 3

needed by ipv6_srh_rcv(), so pskb_pull() in ipv6_srh_rcv() never
fails and can be replaced with skb_pull().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv6/exthdrs.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 6259e907f0d9..e62e26ac99ef 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -402,11 +402,7 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
 
 			skb_postpull_rcsum(skb, skb_network_header(skb),
 					   skb_network_header_len(skb));
-
-			if (!pskb_pull(skb, offset)) {
-				kfree_skb(skb);
-				return -1;
-			}
+			skb_pull(skb, offset);
 			skb_postpull_rcsum(skb, skb_transport_header(skb),
 					   offset);
 
-- 
2.30.2

[PATCH v1 net-next 4/5] ipv6: exthdrs: Reload hdr only when needed in ipv6_srh_rcv().

[Kuniyuki Iwashima]

We need not reload hdr in ipv6_srh_rcv() unless we call
pskb_expand_head().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv6/exthdrs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index e62e26ac99ef..dd23531387e3 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -440,9 +440,9 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
 			kfree_skb(skb);
 			return -1;
 		}
-	}
 
-	hdr = (struct ipv6_sr_hdr *)skb_transport_header(skb);
+		hdr = (struct ipv6_sr_hdr *)skb_transport_header(skb);
+	}
 
 	hdr->segments_left--;
 	addr = hdr->segments + hdr->segments_left;
-- 
2.30.2

[PATCH v1 net-next 5/5] ipv6: exthdrs: Remove redundant skb_headlen() check in ip6_parse_tlv().

[Kuniyuki Iwashima]

ipv6_destopt_rcv() and ipv6_parse_hopopts() pulls these data

  - Hop-by-Hop/Destination Options Header : 8
  - Hdr Ext Len                           : skb_transport_header(skb)[1] << 3

and calls ip6_parse_tlv(), so it need not check if skb_headlen() is less
than skb_transport_offset(skb) + (skb_transport_header(skb)[1] << 3).

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/ipv6/exthdrs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index dd23531387e3..202fc3aaa83c 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -126,9 +126,6 @@ static bool ip6_parse_tlv(bool hopbyhop,
 		max_count = -max_count;
 	}
 
-	if (skb_transport_offset(skb) + len > skb_headlen(skb))
-		goto bad;
-
 	off += 2;
 	len -= 2;
 
-- 
2.30.2

Re: [PATCH v1 net-next 0/5] ipv6: Random cleanup for Extension Header.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 14 Jun 2023 16:01:02 -0700 you wrote:
> This series (1) cleans up pskb_may_pull() in some functions, where needed
> data are already pulled by their caller, (2) removes redundant multicast
> test, and (3) optimises reload timing of the header.
> 
> 
> Kuniyuki Iwashima (5):
>   ipv6: rpl: Remove pskb(_may)?_pull() in ipv6_rpl_srh_rcv().
>   ipv6: rpl: Remove redundant multicast tests in ipv6_rpl_srh_rcv().
>   ipv6: exthdrs: Replace pskb_pull() with skb_pull() in ipv6_srh_rcv().
>   ipv6: exthdrs: Reload hdr only when needed in ipv6_srh_rcv().
>   ipv6: exthdrs: Remove redundant skb_headlen() check in
>     ip6_parse_tlv().
> 
> [...]

Here is the summary with links:
  - [v1,net-next,1/5] ipv6: rpl: Remove pskb(_may)?_pull() in ipv6_rpl_srh_rcv().
    https://git.kernel.org/netdev/net-next/c/ac9d8a66e41d
  - [v1,net-next,2/5] ipv6: rpl: Remove redundant multicast tests in ipv6_rpl_srh_rcv().
    https://git.kernel.org/netdev/net-next/c/6facbca52da2
  - [v1,net-next,3/5] ipv6: exthdrs: Replace pskb_pull() with skb_pull() in ipv6_srh_rcv().
    https://git.kernel.org/netdev/net-next/c/0d2e27b85850
  - [v1,net-next,4/5] ipv6: exthdrs: Reload hdr only when needed in ipv6_srh_rcv().
    https://git.kernel.org/netdev/net-next/c/b83d50f43165
  - [v1,net-next,5/5] ipv6: exthdrs: Remove redundant skb_headlen() check in ip6_parse_tlv().
    https://git.kernel.org/netdev/net-next/c/6db5dd2bf481

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