[PATCH net-next 0/3] net: skb extension follow-ups

[PATCH net-next 0/3] net: skb extension follow-ups

[Paolo Abeni]

This series includes some follow-up for the recently added skb extension.
The first patch addresses an unlikely race while adding skb extensions,
and the following two are just minor code clean-up.

Paolo Abeni (3):
  net: fix possible user-after-free in skb_ext_add()
  net: drop the unused helper skb_ext_get()
  net: minor cleanup in skb_ext_add()

 include/linux/skbuff.h | 11 -----------
 net/core/skbuff.c      | 11 ++++-------
 2 files changed, 4 insertions(+), 18 deletions(-)

-- 
2.19.2

[PATCH net-next 1/3] net: fix possible user-after-free in skb_ext_add()

[Paolo Abeni]

On cow we can free the old extension: we must avoid dereferencing
such extension after skb_ext_maybe_cow(). Since 'new' contents
are always equal to 'old' after the copy, we can fix the above
accessing the relevant data using 'new'.

Fixes: df5042f4c5b9 ("sk_buff: add skb extension infrastructure")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/core/skbuff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index cb0bf4215745..e1d88762f659 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5666,13 +5666,13 @@ void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id)
 		if (!new)
 			return NULL;
 
-		if (__skb_ext_exist(old, id)) {
+		if (__skb_ext_exist(new, id)) {
 			if (old != new)
 				skb->extensions = new;
 			goto set_active;
 		}
 
-		newoff = old->chunks;
+		newoff = new->chunks;
 	} else {
 		newoff = SKB_EXT_CHUNKSIZEOF(*new);
 
-- 
2.19.2

Re: [PATCH net-next 1/3] net: fix possible user-after-free in skb_ext_add()

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> On cow we can free the old extension: we must avoid dereferencing
> such extension after skb_ext_maybe_cow(). Since 'new' contents
> are always equal to 'old' after the copy, we can fix the above
> accessing the relevant data using 'new'.

Yep, this is indeed possible when two clones of same skb
have skb_ext_add() invoked at the same time from two different
context.

Thanks for spotting this.

Acked-by: Florian Westphal <fw@strlen.de>

[PATCH net-next 2/3] net: drop the unused helper skb_ext_get()

[Paolo Abeni]

Such helper is currently unused, and skb extension users are
better off using skb_ext_add()/skb_ext_del(). So let's drop
it.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/linux/skbuff.h | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 3f741b04e55d..2a57a365c711 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3938,16 +3938,6 @@ static inline void skb_ext_put(struct sk_buff *skb)
 		__skb_ext_put(skb->extensions);
 }
 
-static inline void skb_ext_get(struct sk_buff *skb)
-{
-	if (skb->active_extensions) {
-		struct skb_ext *ext = skb->extensions;
-
-		if (ext)
-			refcount_inc(&ext->refcnt);
-	}
-}
-
 static inline void __skb_ext_copy(struct sk_buff *dst,
 				  const struct sk_buff *src)
 {
@@ -3995,7 +3985,6 @@ static inline void *skb_ext_find(const struct sk_buff *skb, enum skb_ext_id id)
 }
 #else
 static inline void skb_ext_put(struct sk_buff *skb) {}
-static inline void skb_ext_get(struct sk_buff *skb) {}
 static inline void skb_ext_del(struct sk_buff *skb, int unused) {}
 static inline void __skb_ext_copy(struct sk_buff *d, const struct sk_buff *s) {}
 static inline void skb_ext_copy(struct sk_buff *dst, const struct sk_buff *s) {}
-- 
2.19.2

Re: [PATCH net-next 2/3] net: drop the unused helper skb_ext_get()

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> Such helper is currently unused, and skb extension users are
> better off using skb_ext_add()/skb_ext_del(). So let's drop
> it.

Right, I had a look at one of my older branches and its not used
there either.

Acked-by: Florian Westphal <fw@strlen.de>

[PATCH net-next 3/3] net: minor cleanup in skb_ext_add()

[Paolo Abeni]

When the extension to be added is already present, the only
skb field we may need to update is 'extensions': we can reorder
the code and avoid a branch.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/core/skbuff.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e1d88762f659..38afa3ff2b44 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5666,11 +5666,8 @@ void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id)
 		if (!new)
 			return NULL;
 
-		if (__skb_ext_exist(new, id)) {
-			if (old != new)
-				skb->extensions = new;
+		if (__skb_ext_exist(new, id))
 			goto set_active;
-		}
 
 		newoff = new->chunks;
 	} else {
@@ -5684,9 +5681,9 @@ void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id)
 	newlen = newoff + skb_ext_type_len[id];
 	new->chunks = newlen;
 	new->offset[id] = newoff;
-	skb->extensions = new;
-set_active:
 	skb->active_extensions |= 1 << id;
+set_active:
+	skb->extensions = new;
 	return skb_ext_get_ptr(new, id);
 }
 EXPORT_SYMBOL(skb_ext_add);
-- 
2.19.2

Re: [PATCH net-next 3/3] net: minor cleanup in skb_ext_add()

[Florian Westphal]

Paolo Abeni <pabeni@redhat.com> wrote:
> When the extension to be added is already present, the only
> skb field we may need to update is 'extensions': we can reorder
> the code and avoid a branch.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/core/skbuff.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index e1d88762f659..38afa3ff2b44 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5666,11 +5666,8 @@ void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id)
>  		if (!new)
>  			return NULL;
>  
> -		if (__skb_ext_exist(new, id)) {
> -			if (old != new)
> -				skb->extensions = new;

Yes, the old != new check can be avoided, we would
replace "extensions" with the same content.

> +		if (__skb_ext_exist(new, id))
>  			goto set_active;
> -		}
>  
>  		newoff = new->chunks;
>  	} else {
> @@ -5684,9 +5681,9 @@ void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id)
>  	newlen = newoff + skb_ext_type_len[id];
>  	new->chunks = newlen;
>  	new->offset[id] = newoff;
> -	skb->extensions = new;
> -set_active:
>  	skb->active_extensions |= 1 << id;
> +set_active:
> +	skb->extensions = new;

Why are you moving the label?
I don't think thats correct.

We must make sure the id is flagged as active in
active_extensions.

Re: [PATCH net-next 0/3] net: skb extension follow-ups

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Thu, 20 Dec 2018 19:38:15 +0100

> This series includes some follow-up for the recently added skb extension.
> The first patch addresses an unlikely race while adding skb extensions,
> and the following two are just minor code clean-up.

Please respin after addressing Florian's feedback on patch #3.

Thanks!

Re: [PATCH net-next 0/3] net: skb extension follow-ups

[Paolo Abeni]

On Thu, 2018-12-20 at 16:51 -0800, David Miller wrote:
> From: Paolo Abeni <pabeni@redhat.com>
> Date: Thu, 20 Dec 2018 19:38:15 +0100
> 
> > This series includes some follow-up for the recently added skb extension.
> > The first patch addresses an unlikely race while adding skb extensions,
> > and the following two are just minor code clean-up.
> 
> Please respin after addressing Florian's feedback on patch #3.

I'm sorry for the not so prompt reply.

I will do/I'm doing that, but this is net-next material, so I guess it
has to wait, right?

Thank you,

Paolo

Re: [PATCH net-next 0/3] net: skb extension follow-ups

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Fri, 21 Dec 2018 13:09:34 +0100

> On Thu, 2018-12-20 at 16:51 -0800, David Miller wrote:
>> From: Paolo Abeni <pabeni@redhat.com>
>> Date: Thu, 20 Dec 2018 19:38:15 +0100
>> 
>> > This series includes some follow-up for the recently added skb extension.
>> > The first patch addresses an unlikely race while adding skb extensions,
>> > and the following two are just minor code clean-up.
>> 
>> Please respin after addressing Florian's feedback on patch #3.
> 
> I'm sorry for the not so prompt reply.
> 
> I will do/I'm doing that, but this is net-next material, so I guess it
> has to wait, right?

Please send it to me.