From: Daniel Borkmann <daniel@iogearbox•net>
To: David Miller <davem@davemloft•net>
Cc: chamaken@gmail•com, fw@strlen•de, netdev@vger•kernel.org
Subject: Re: [PATCH net] netlink, mmap: transform mmap skb into full skb on taps
Date: Fri, 11 Sep 2015 12:25:45 +0200 [thread overview]
Message-ID: <55F2AC29.3030209@iogearbox.net> (raw)
In-Reply-To: <20150910.221132.1962916984876023271.davem@davemloft.net>
On 09/11/2015 07:11 AM, David Miller wrote:
...
> Looking more deeply into this, I think we have the same exact problem
> with netlink skbs that use vmalloc memory at skb->head.
Yes, agreed, the test in the patch covered those as well via:
if (netlink_skb_is_mmaped(skb) || is_vmalloc_addr(skb->head))
> We have a special hack, but only when netlink_skb_clone() is used,
> to preserve the destructor. But these skbs can escape anywhere
> and be eventually cloned as we have seen with the mmap stuff.
Yes, it looks like they are currently only used from user space to
kernel space. I saw that 3a36515f7294 ("netlink: fix splat in skb_clone
with large messages") fixed a couple of these in upper layers with
regards to large skbs, so there's a chance that this can be overseen
rather easily as well in other places and then only come to light in
cases where we allocate more than NLMSG_GOODSIZE, so we don't actually
use the alloc_skb() path. :/ So I like your idea below!
> I'm wondering if we should do something more precise to fix this,
> and in a way that handles both the mmap and vmalloc cases.
Perhaps it might also be useful if the kernel would one day want to
use netlink_alloc_large_skb() for answers back to user space, or in
places where we use network skbs (haven't looked into it with regards
to this).
Some more comments below:
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 2738d35..77b804c 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -584,8 +584,9 @@ struct sk_buff {
> fclone:2,
> peeked:1,
> head_frag:1,
> - xmit_more:1;
> - /* one bit hole */
> + xmit_more:1,
> + clone_preserves_destructor;
( Nit: maybe as clone_preserves_destructor:1 )
> +
> kmemcheck_bitfield_end(flags1);
>
> /* fields enclosed in headers_start/headers_end are copied
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index dad4dd3..4a7b8e3 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -825,7 +825,8 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
> n->hdr_len = skb->nohdr ? skb_headroom(skb) : skb->hdr_len;
> n->cloned = 1;
> n->nohdr = 0;
> - n->destructor = NULL;
> + if (!skb->clone_preserves_destructor)
> + n->destructor = NULL;
I think we also need here:
else
C(destructor);
> C(tail);
> C(end);
> C(head);
[...]
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 7f86d3b..214f1a1 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -719,6 +719,7 @@ static void netlink_ring_setup_skb(struct sk_buff *skb, struct sock *sk,
> skb->end = skb->tail + size;
> skb->len = 0;
>
> + skb->clone_preserves_destructor = 1;
> skb->destructor = netlink_skb_destructor;
> NETLINK_CB(skb).flags |= NETLINK_SKB_MMAPED;
> NETLINK_CB(skb).sk = sk;
> @@ -854,6 +855,14 @@ static void netlink_ring_set_copied(struct sock *sk, struct sk_buff *skb)
> #define netlink_mmap_sendmsg(sk, msg, dst_portid, dst_group, scm) 0
> #endif /* CONFIG_NETLINK_MMAP */
One more thing that came to my mind. For netlink mmap skbs, the
skb->clone_preserves_destructor is actually not enough.
Already calling into skb_clone() is an issue itself, as the data area is
user space buffer, and skb_clone() as well as skb_copy() access skb_shinfo()
area. :/ So in that regard netlink mmap skbs are even further restrictive
on what we can do than netlink large skbs.
Best,
Daniel
next prev parent reply other threads:[~2015-09-11 10:25 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-10 18:05 [PATCH net] netlink, mmap: transform mmap skb into full skb on taps Daniel Borkmann
2015-09-11 5:11 ` David Miller
2015-09-11 10:25 ` Daniel Borkmann [this message]
2015-09-11 19:42 ` David Miller
2015-09-11 20:35 ` Daniel Borkmann
2015-09-11 21:34 ` David Miller
2015-09-11 22:18 ` Daniel Borkmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55F2AC29.3030209@iogearbox.net \
--to=daniel@iogearbox$(echo .)net \
--cc=chamaken@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=fw@strlen$(echo .)de \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox