* [PATCH net-next 0/3] net: skb extension follow-ups
@ 2018-12-20 18:38 Paolo Abeni
2018-12-20 18:38 ` [PATCH net-next 1/3] net: fix possible user-after-free in skb_ext_add() Paolo Abeni
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Paolo Abeni @ 2018-12-20 18:38 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal, David S. Miller
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next 1/3] net: fix possible user-after-free in skb_ext_add()
2018-12-20 18:38 [PATCH net-next 0/3] net: skb extension follow-ups Paolo Abeni
@ 2018-12-20 18:38 ` Paolo Abeni
2018-12-20 19:32 ` Florian Westphal
2018-12-20 18:38 ` [PATCH net-next 2/3] net: drop the unused helper skb_ext_get() Paolo Abeni
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Paolo Abeni @ 2018-12-20 18:38 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal, David S. Miller
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next 2/3] net: drop the unused helper skb_ext_get()
2018-12-20 18:38 [PATCH net-next 0/3] net: skb extension follow-ups Paolo Abeni
2018-12-20 18:38 ` [PATCH net-next 1/3] net: fix possible user-after-free in skb_ext_add() Paolo Abeni
@ 2018-12-20 18:38 ` Paolo Abeni
2018-12-20 19:30 ` Florian Westphal
2018-12-20 18:38 ` [PATCH net-next 3/3] net: minor cleanup in skb_ext_add() Paolo Abeni
2018-12-21 0:51 ` [PATCH net-next 0/3] net: skb extension follow-ups David Miller
3 siblings, 1 reply; 10+ messages in thread
From: Paolo Abeni @ 2018-12-20 18:38 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal, David S. Miller
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next 3/3] net: minor cleanup in skb_ext_add()
2018-12-20 18:38 [PATCH net-next 0/3] net: skb extension follow-ups Paolo Abeni
2018-12-20 18:38 ` [PATCH net-next 1/3] net: fix possible user-after-free in skb_ext_add() Paolo Abeni
2018-12-20 18:38 ` [PATCH net-next 2/3] net: drop the unused helper skb_ext_get() Paolo Abeni
@ 2018-12-20 18:38 ` Paolo Abeni
2018-12-20 19:29 ` Florian Westphal
2018-12-21 0:51 ` [PATCH net-next 0/3] net: skb extension follow-ups David Miller
3 siblings, 1 reply; 10+ messages in thread
From: Paolo Abeni @ 2018-12-20 18:38 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal, David S. Miller
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 3/3] net: minor cleanup in skb_ext_add()
2018-12-20 18:38 ` [PATCH net-next 3/3] net: minor cleanup in skb_ext_add() Paolo Abeni
@ 2018-12-20 19:29 ` Florian Westphal
0 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2018-12-20 19:29 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, Florian Westphal, David S. Miller
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 2/3] net: drop the unused helper skb_ext_get()
2018-12-20 18:38 ` [PATCH net-next 2/3] net: drop the unused helper skb_ext_get() Paolo Abeni
@ 2018-12-20 19:30 ` Florian Westphal
0 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2018-12-20 19:30 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, Florian Westphal, David S. Miller
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 1/3] net: fix possible user-after-free in skb_ext_add()
2018-12-20 18:38 ` [PATCH net-next 1/3] net: fix possible user-after-free in skb_ext_add() Paolo Abeni
@ 2018-12-20 19:32 ` Florian Westphal
0 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2018-12-20 19:32 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, Florian Westphal, David S. Miller
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 0/3] net: skb extension follow-ups
2018-12-20 18:38 [PATCH net-next 0/3] net: skb extension follow-ups Paolo Abeni
` (2 preceding siblings ...)
2018-12-20 18:38 ` [PATCH net-next 3/3] net: minor cleanup in skb_ext_add() Paolo Abeni
@ 2018-12-21 0:51 ` David Miller
2018-12-21 12:09 ` Paolo Abeni
3 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2018-12-21 0:51 UTC (permalink / raw)
To: pabeni; +Cc: netdev, fw
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!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 0/3] net: skb extension follow-ups
2018-12-21 0:51 ` [PATCH net-next 0/3] net: skb extension follow-ups David Miller
@ 2018-12-21 12:09 ` Paolo Abeni
2018-12-21 16:55 ` David Miller
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Abeni @ 2018-12-21 12:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev, fw
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 0/3] net: skb extension follow-ups
2018-12-21 12:09 ` Paolo Abeni
@ 2018-12-21 16:55 ` David Miller
0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2018-12-21 16:55 UTC (permalink / raw)
To: pabeni; +Cc: netdev, fw
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-12-21 16:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-20 18:38 [PATCH net-next 0/3] net: skb extension follow-ups Paolo Abeni
2018-12-20 18:38 ` [PATCH net-next 1/3] net: fix possible user-after-free in skb_ext_add() Paolo Abeni
2018-12-20 19:32 ` Florian Westphal
2018-12-20 18:38 ` [PATCH net-next 2/3] net: drop the unused helper skb_ext_get() Paolo Abeni
2018-12-20 19:30 ` Florian Westphal
2018-12-20 18:38 ` [PATCH net-next 3/3] net: minor cleanup in skb_ext_add() Paolo Abeni
2018-12-20 19:29 ` Florian Westphal
2018-12-21 0:51 ` [PATCH net-next 0/3] net: skb extension follow-ups David Miller
2018-12-21 12:09 ` Paolo Abeni
2018-12-21 16:55 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).