From: Zoltan Kiss <zoltan.kiss@citrix.com>
To: Ben Hutchings <ben@decadent.org.uk>
Cc: Luis Henriques <luis.henriques@canonical.com>,
David Miller <davem@davemloft.net>, <jesse@nicira.com>,
<pshelar@nicira.com>, <tgraf@redhat.com>, <dev@openvswitch.org>,
<pablo@netfilter.org>, <kadlec@blackhole.kfki.hu>,
<edumazet@google.com>, <dborkman@redhat.com>,
<therbert@google.com>, <jasowang@redhat.com>, <fw@strlen.de>,
<joe@perches.com>, <horms@verge.net.au>, <jiri@resnulli.us>,
<mst@redhat.com>, <Paul.Durrant@citrix.com>, <JBeulich@suse.com>,
<herbert@gondor.apana.org.au>, <mszeredi@suse.cz>,
<netfilter-devel@vger.kernel.org>, <netfilter@vger.kernel.org>,
<xen-devel@lists.xenproject.org>, <netdev@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <kvm@vger.kernel.org>
Subject: Re: [PATCH 3.13] core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors
Date: Wed, 23 Apr 2014 13:34:10 +0100 [thread overview]
Message-ID: <5357B342.9090204@citrix.com> (raw)
In-Reply-To: <1398194284.7767.101.camel@deadeye.wl.decadent.org.uk>
On 22/04/14 20:18, Ben Hutchings wrote:
> From: Zoltan Kiss <zoltan.kiss@citrix.com>
>
> commit 36d5fe6a000790f56039afe26834265db0a3ad4c upstream.
>
> skb_zerocopy can copy elements of the frags array between skbs, but it doesn't
> orphan them. Also, it doesn't handle errors, so this patch takes care of that
> as well, and modify the callers accordingly. skb_tx_error() is also added to
> the callers so they will signal the failed delivery towards the creator of the
> skb.
>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [bwh: Backported to 3.13: skb_zerocopy() is new in 3.14, but was moved from a
> static function in nfnetlink_queue. We need to patch that and its caller, but
> not openvswitch.]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> On Tue, 2014-04-22 at 19:02 +0100, Zoltan Kiss wrote:
>> On 22/04/14 16:38, Ben Hutchings wrote:
>>> On Mon, 2014-04-21 at 12:26 +0100, Luis Henriques wrote:
>>>> Hi David,
>>>>
>>>> On Thu, Mar 27, 2014 at 03:29:56PM -0400, David Miller wrote:
>>>>> From: Zoltan Kiss <zoltan.kiss@citrix.com>
>>>>> Date: Wed, 26 Mar 2014 22:37:45 +0000
>>>>>
>>>>>> skb_zerocopy can copy elements of the frags array between skbs, but it doesn't
>>>>>> orphan them. Also, it doesn't handle errors, so this patch takes care of that
>>>>>> as well, and modify the callers accordingly. skb_tx_error() is also added to
>>>>>> the callers so they will signal the failed delivery towards the creator of the
>>>>>> skb.
>>>>>>
>>>>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>>>>>
>>>>> Fingers crossed :-) Applied, thanks Zoltan.
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>> Could you please queue this for stable as well? It has CVE-2014-2568
>>>> assigned to it and I believe it is applicable to some of the trees.
>>>
>>> It was applied to 3.13, but needs backporting to earlier versions. I
>>> posted my attempt in
>>> <1397429860.10849.86.camel@deadeye.wl.decadent.org.uk> but it needs
>>> testing/reviewing.
>>
>> This one is a different issue, although it is very similar.
>
> Sorry for mixing these up. I believe this bug has been present since
> zerocopy was added to nfqueue in Linux 3.10 (commit ae08ce002108). This
> is the version I used for Debian's 3.13 branch, which might be usable
> for older stable branches too.
Seems OK, thanks!
>
> Ben.
>
> ---
> --- a/net/netfilter/nfnetlink_queue_core.c
> +++ b/net/netfilter/nfnetlink_queue_core.c
> @@ -235,22 +235,23 @@ nfqnl_flush(struct nfqnl_instance *queue
> spin_unlock_bh(&queue->lock);
> }
>
> -static void
> +static int
> nfqnl_zcopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
> {
> int i, j = 0;
> int plen = 0; /* length of skb->head fragment */
> + int ret;
> struct page *page;
> unsigned int offset;
>
> /* dont bother with small payloads */
> - if (len <= skb_tailroom(to)) {
> - skb_copy_bits(from, 0, skb_put(to, len), len);
> - return;
> - }
> + if (len <= skb_tailroom(to))
> + return skb_copy_bits(from, 0, skb_put(to, len), len);
>
> if (hlen) {
> - skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
> + ret = skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
> + if (unlikely(ret))
> + return ret;
> len -= hlen;
> } else {
> plen = min_t(int, skb_headlen(from), len);
> @@ -268,6 +269,11 @@ nfqnl_zcopy(struct sk_buff *to, const st
> to->len += len + plen;
> to->data_len += len + plen;
>
> + if (unlikely(skb_orphan_frags(from, GFP_ATOMIC))) {
> + skb_tx_error(from);
> + return -ENOMEM;
> + }
> +
> for (i = 0; i < skb_shinfo(from)->nr_frags; i++) {
> if (!len)
> break;
> @@ -278,6 +284,8 @@ nfqnl_zcopy(struct sk_buff *to, const st
> j++;
> }
> skb_shinfo(to)->nr_frags = j;
> +
> + return 0;
> }
>
> static int
> @@ -374,13 +382,16 @@ nfqnl_build_packet_message(struct net *n
>
> skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
> GFP_ATOMIC);
> - if (!skb)
> + if (!skb) {
> + skb_tx_error(entskb);
> return NULL;
> + }
>
> nlh = nlmsg_put(skb, 0, 0,
> NFNL_SUBSYS_QUEUE << 8 | NFQNL_MSG_PACKET,
> sizeof(struct nfgenmsg), 0);
> if (!nlh) {
> + skb_tx_error(entskb);
> kfree_skb(skb);
> return NULL;
> }
> @@ -504,13 +515,15 @@ nfqnl_build_packet_message(struct net *n
> nla->nla_type = NFQA_PAYLOAD;
> nla->nla_len = nla_attr_size(data_len);
>
> - nfqnl_zcopy(skb, entskb, data_len, hlen);
> + if (nfqnl_zcopy(skb, entskb, data_len, hlen))
> + goto nla_put_failure;
> }
>
> nlh->nlmsg_len = skb->len;
> return skb;
>
> nla_put_failure:
> + skb_tx_error(entskb);
> kfree_skb(skb);
> net_err_ratelimited("nf_queue: error creating packet message\n");
> return NULL;
>
next prev parent reply other threads:[~2014-04-23 12:34 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-26 22:37 [PATCH v5] core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors Zoltan Kiss
2014-03-27 19:29 ` David Miller
2014-04-21 11:26 ` Luis Henriques
2014-04-22 15:38 ` Ben Hutchings
2014-04-22 18:02 ` Zoltan Kiss
2014-04-22 19:18 ` [PATCH 3.13] " Ben Hutchings
2014-04-23 8:57 ` Luis Henriques
2014-04-23 12:34 ` Zoltan Kiss [this message]
2014-04-23 13:31 ` Josh Boyer
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=5357B342.9090204@citrix.com \
--to=zoltan.kiss@citrix.com \
--cc=JBeulich@suse.com \
--cc=Paul.Durrant@citrix.com \
--cc=ben@decadent.org.uk \
--cc=davem@davemloft.net \
--cc=dborkman@redhat.com \
--cc=dev@openvswitch.org \
--cc=edumazet@google.com \
--cc=fw@strlen.de \
--cc=herbert@gondor.apana.org.au \
--cc=horms@verge.net.au \
--cc=jasowang@redhat.com \
--cc=jesse@nicira.com \
--cc=jiri@resnulli.us \
--cc=joe@perches.com \
--cc=kadlec@blackhole.kfki.hu \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luis.henriques@canonical.com \
--cc=mst@redhat.com \
--cc=mszeredi@suse.cz \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=netfilter@vger.kernel.org \
--cc=pablo@netfilter.org \
--cc=pshelar@nicira.com \
--cc=tgraf@redhat.com \
--cc=therbert@google.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).