Skip to contentSkip to conversation

net: skb extension follow-ups

PATCH In review Paolo Abeni 3 participants 6 messages Dec 21, 2018

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Paolo Abeni

author maintainer
#0 lore raw

v2 cover net: skb extension follow-ups

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.

v1 -> v2:
- be sure to flag the newly added extension as active in skb_ext_add()

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      |  9 +++------
 2 files changed, 3 insertions(+), 17 deletions(-)
Signature · 1 line
2.19.2
author maintainer
#1 lore raw

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

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 <[email protected]>
Acked-by Florian Westphal <[email protected]>
---
 net/core/skbuff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/core/skbuff.c +2 -2
index cb0bf4215745..e1d88762f659 100644
@@ -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);
Signature · 1 line
2.19.2
author maintainer
#2 lore raw

PATCH v2 2/3 net: drop the unused helper skb_ext_get()

In reply to 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 <[email protected]>
Acked-by Florian Westphal <[email protected]>
---
 include/linux/skbuff.h | 11 -----------
 1 file changed, 11 deletions(-)
include/linux/skbuff.h +0 -11
index 3f741b04e55d..2a57a365c711 100644
@@ -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) {}
Signature · 1 line
2.19.2
author maintainer
#3 lore raw

PATCH v2 3/3 net: minor cleanup in skb_ext_add()

In reply to 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.

v1 -> v2:
- be sure to flag the newly added extension as active

Signed-off-by Paolo Abeni <[email protected]>
---
 net/core/skbuff.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
net/core/skbuff.c +2 -5
index e1d88762f659..37317ffec146 100644
@@ -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,8 +5681,8 @@ 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->extensions = new;
 	skb->active_extensions |= 1 << id;
 	return skb_ext_get_ptr(new, id);
 }
Signature · 1 line
2.19.2
Acked-by on v2 3/3
#4 lore raw

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

Quoted 3 lines · 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.

Thanks Paolo.

Acked-by Florian Westphal <[email protected]>
maintainer
#5 lore raw
In reply to Paolo Abeni

From: Paolo Abeni <[email protected]>
Date: Fri, 21 Dec 2018 19:03:12 +0100

Quoted 6 lines

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.

v1 -> v2:
- be sure to flag the newly added extension as active in skb_ext_add()

Series applied, thanks.