[{"id":1764213,"web_url":"http://patchwork.ozlabs.org/comment/1764213/","msgid":"","list_archive_url":null,"date":"2017-09-06T15:44:18","subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","submitter":{"id":971,"url":"http://patchwork.ozlabs.org/api/people/971/","name":"Andy Gospodarek","email":"andy@greyhouse.net"},"content":"On Wed, Sep 6, 2017 at 11:26 AM, Jesper Dangaard Brouer\n wrote:\n> Using bpf_redirect_map is allowed for generic XDP programs, but the\n> appropriate map lookup was never performed in xdp_do_generic_redirect().\n>\n> Instead the map-index is directly used as the ifindex. For the\n> xdp_redirect_map sample in SKB-mode '-S', this resulted in trying\n> sending on ifindex 0 which isn't valid, resulting in getting SKB\n> packets dropped. Thus, the reported performance numbers are wrong in\n> commit 24251c264798 (\"samples/bpf: add option for native and skb mode\n> for redirect apps\") for the 'xdp_redirect_map -S' case.\n>\n> It might seem innocent this was lacking, but it can actually crash the\n> kernel. The potential crash is caused by not consuming redirect_info->map.\n> The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map\n> pointer, which will survive even after unloading the xdp bpf_prog and\n> deallocating the devmap data-structure. This leaves a dead map\n> pointer around. The kernel will crash when loading the xdp_redirect\n> sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)\n> and returns XDP_REDIRECT, which will cause it to dereference the map\n> pointer.\n\nNice catch!\n\nSince 'net-next' is closed and this is a bugfix it seems like this is\na good candidate for 'net' right?\n\n>\n> Fixes: 6103aa96ec07 (\"net: implement XDP_REDIRECT for xdp generic\")\n> Fixes: 24251c264798 (\"samples/bpf: add option for native and skb mode for redirect apps\")\n> Signed-off-by: Jesper Dangaard Brouer \n\nAcked-by: Andy Gospodarek \n\n\n> ---\n> net/core/filter.c | 29 +++++++++++++++++++++++++++++\n> 1 file changed, 29 insertions(+)\n>\n> diff --git a/net/core/filter.c b/net/core/filter.c\n> index 5912c738a7b2..6a4745bf2c9f 100644\n> --- a/net/core/filter.c\n> +++ b/net/core/filter.c\n> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,\n> }\n> EXPORT_SYMBOL_GPL(xdp_do_redirect);\n>\n> +static int xdp_do_generic_redirect_map(struct net_device *dev,\n> + struct sk_buff *skb,\n> + struct bpf_prog *xdp_prog)\n> +{\n> + struct redirect_info *ri = this_cpu_ptr(&redirect_info);\n> + struct bpf_map *map = ri->map;\n> + u32 index = ri->ifindex;\n> + struct net_device *fwd;\n> + int err;\n> +\n> + ri->ifindex = 0;\n> + ri->map = NULL;\n> +\n> + fwd = __dev_map_lookup_elem(map, index);\n> + if (!fwd) {\n> + err = -EINVAL;\n> + goto err;\n> + }\n> + skb->dev = fwd;\n> + _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);\n> + return 0;\n> +err:\n> + _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);\n> + return err;\n> +}\n> +\n> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n> struct bpf_prog *xdp_prog)\n> {\n> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n> unsigned int len;\n> int err = 0;\n>\n> + if (ri->map)\n> + return xdp_do_generic_redirect_map(dev, skb, xdp_prog);\n> +\n> fwd = dev_get_by_index_rcu(dev_net(dev), index);\n> ri->ifindex = 0;\n> if (unlikely(!fwd)) {\n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=greyhouse-net.20150623.gappssmtp.com\n\theader.i=@greyhouse-net.20150623.gappssmtp.com\n\theader.b=\"HiaY2lbs\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnSZS1xDtz9s81\n\tfor ;\n\tThu, 7 Sep 2017 01:44:48 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754912AbdIFPop (ORCPT );\n\tWed, 6 Sep 2017 11:44:45 -0400","from mail-qt0-f193.google.com ([209.85.216.193]:33373 \"EHLO\n\tmail-qt0-f193.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1754902AbdIFPok (ORCPT\n\t); Wed, 6 Sep 2017 11:44:40 -0400","by mail-qt0-f193.google.com with SMTP id h15so4127033qta.0\n\tfor ; Wed, 06 Sep 2017 08:44:39 -0700 (PDT)","by 10.12.173.52 with HTTP; Wed, 6 Sep 2017 08:44:18 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=greyhouse-net.20150623.gappssmtp.com; s=20150623;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=Sow4FQHvzbAruk3SlcoE+J7tHPf3mPnt4Qyx2ITxvdc=;\n\tb=HiaY2lbsY5ZdFSu/Jp20vrk6onpmIX6RrZCPKIJkHHGe+xRKWoc9uoTQDpacefk43O\n\tNp4ebvbNcHAKGCz7rCi9g5t7QJ0/AulQYcyap5ue+3iGcog1TxtsRUMsV4XKmNezCJfH\n\tHKOF2su7NpF/paOKX9R4OvWt0JhJKFd0H1YmQKycPD1PSFY3EbVp99T4UOMGlhgzISZr\n\twfV9luGjnRdeImS3ge/XsODZ2z7Lqsziz6LPdmylB1bpKTWo+QfDPsTfdE6YhxuzVL3q\n\tdd1WXqaEVrhGlHsMIvk23dDsjScoZtb8EUQZGPgUP23a/9qYXqpQTi5PskAgnKQf2bVq\n\t1QPA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=Sow4FQHvzbAruk3SlcoE+J7tHPf3mPnt4Qyx2ITxvdc=;\n\tb=UAT51PBcz6XsEqre82H5UXMe24+0w4d1PhhdhiYAKRzSwmpQ4UV6Z7Um1/mjYgRlPx\n\tn8V40XFjshvUPc/WKNX8ARyYpsx7SHc+H3eldLBPY33aG4TL+ZPT0Xnhfl9hwLjvji5b\n\t/SYCNNvsUV14GKgBp22IQpu34cLQqLYZf1iXSqIucSIW/s4tY0PnZ6ejDXkupimCuPPu\n\tY3GDweCo6QZsYJ6V67V5icKp0zQFrKL/WEM2VJpLZw2AiU4xwYZaAGuZZZbks2XjoCTV\n\t80ODXAqD/LMrdedt7781RsduVajqMEJKTOgwIQcAuI3ol7lRWfRg4f+vm/8mA8Qwi2gg\n\tsYog==","X-Gm-Message-State":"AHPjjUi30svberk2qTbFO/iM8xPSOl3VOCiZA+UkvzzhUbgsJXhTiC/v\n\tUoiUhxsOnqNrfzbsMoTcqHEpJexzxHTGZuY=","X-Google-Smtp-Source":"ADKCNb6VxN+4MINbFf70ldLUkBAiclg+lXLgzGw5/GCElybp0EV+yS/Yz6n88fbQGOlhyuCmVBvPP+2641uIWIeh6tg=","X-Received":"by 10.237.63.153 with SMTP id s25mr4163204qth.134.1504712679273; \n\tWed, 06 Sep 2017 08:44:39 -0700 (PDT)","MIME-Version":"1.0","X-Originating-IP":"[192.19.231.250]","In-Reply-To":"<150471158528.3727.12324542627400287360.stgit@firesoul>","References":"<150471158528.3727.12324542627400287360.stgit@firesoul>","From":"Andy Gospodarek ","Date":"Wed, 6 Sep 2017 11:44:18 -0400","Message-ID":"","Subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","To":"Jesper Dangaard Brouer ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"David S. Miller\" ,\n\tJohn Fastabend ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1764221,"web_url":"http://patchwork.ozlabs.org/comment/1764221/","msgid":"<20170906180139.1764ca77@redhat.com>","list_archive_url":null,"date":"2017-09-06T16:01:39","subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","submitter":{"id":13625,"url":"http://patchwork.ozlabs.org/api/people/13625/","name":"Jesper Dangaard Brouer","email":"brouer@redhat.com"},"content":"On Wed, 6 Sep 2017 11:44:18 -0400\nAndy Gospodarek wrote:\n\n> On Wed, Sep 6, 2017 at 11:26 AM, Jesper Dangaard Brouer\n> wrote:\n> > Using bpf_redirect_map is allowed for generic XDP programs, but the\n> > appropriate map lookup was never performed in xdp_do_generic_redirect().\n> >\n> > Instead the map-index is directly used as the ifindex. For the\n> > xdp_redirect_map sample in SKB-mode '-S', this resulted in trying\n> > sending on ifindex 0 which isn't valid, resulting in getting SKB\n> > packets dropped. Thus, the reported performance numbers are wrong in\n> > commit 24251c264798 (\"samples/bpf: add option for native and skb mode\n> > for redirect apps\") for the 'xdp_redirect_map -S' case.\n> >\n> > It might seem innocent this was lacking, but it can actually crash the\n> > kernel. The potential crash is caused by not consuming redirect_info->map.\n> > The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map\n> > pointer, which will survive even after unloading the xdp bpf_prog and\n> > deallocating the devmap data-structure. This leaves a dead map\n> > pointer around. The kernel will crash when loading the xdp_redirect\n> > sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)\n> > and returns XDP_REDIRECT, which will cause it to dereference the map\n> > pointer. \n> \n> Nice catch!\n> \n> Since 'net-next' is closed and this is a bugfix it seems like this is\n> a good candidate for 'net' right?\n\nYes, I know 'net-next' is closed, but 'net' doesn't contain the\nXDP_REDIRECT code yet... thus I had to base it on net-next ;-)\n\n\n> >\n> > Fixes: 6103aa96ec07 (\"net: implement XDP_REDIRECT for xdp generic\")\n> > Fixes: 24251c264798 (\"samples/bpf: add option for native and skb mode for redirect apps\")\n> > Signed-off-by: Jesper Dangaard Brouer \n> \n> Acked-by: Andy Gospodarek \n\nThanks","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=brouer@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnSy538nmz9t4Z\n\tfor ;\n\tThu, 7 Sep 2017 02:01:49 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754098AbdIFQBq (ORCPT );\n\tWed, 6 Sep 2017 12:01:46 -0400","from mx1.redhat.com ([209.132.183.28]:34762 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1753586AbdIFQBp (ORCPT );\n\tWed, 6 Sep 2017 12:01:45 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 9C9DD80090;\n\tWed, 6 Sep 2017 16:01:45 +0000 (UTC)","from localhost (ovpn-200-27.brq.redhat.com [10.40.200.27])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id D91AD1980A;\n\tWed, 6 Sep 2017 16:01:40 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 9C9DD80090","Date":"Wed, 6 Sep 2017 18:01:39 +0200","From":"Jesper Dangaard Brouer ","To":"Andy Gospodarek ","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"David S. Miller\" ,\n\tJohn Fastabend , brouer@redhat.com","Subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","Message-ID":"<20170906180139.1764ca77@redhat.com>","In-Reply-To":"","References":"<150471158528.3727.12324542627400287360.stgit@firesoul>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.28]);\n\tWed, 06 Sep 2017 16:01:45 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1764241,"web_url":"http://patchwork.ozlabs.org/comment/1764241/","msgid":"<59B02127.5020904@iogearbox.net>","list_archive_url":null,"date":"2017-09-06T16:24:07","subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","submitter":{"id":65705,"url":"http://patchwork.ozlabs.org/api/people/65705/","name":"Daniel Borkmann","email":"daniel@iogearbox.net"},"content":"On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:\n> Using bpf_redirect_map is allowed for generic XDP programs, but the\n> appropriate map lookup was never performed in xdp_do_generic_redirect().\n>\n> Instead the map-index is directly used as the ifindex. For the\n\nGood point, but ...\n\n[...]\n> net/core/filter.c | 29 +++++++++++++++++++++++++++++\n> 1 file changed, 29 insertions(+)\n>\n> diff --git a/net/core/filter.c b/net/core/filter.c\n> index 5912c738a7b2..6a4745bf2c9f 100644\n> --- a/net/core/filter.c\n> +++ b/net/core/filter.c\n> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,\n> }\n> EXPORT_SYMBOL_GPL(xdp_do_redirect);\n>\n> +static int xdp_do_generic_redirect_map(struct net_device *dev,\n> +\t\t\t\t struct sk_buff *skb,\n> +\t\t\t\t struct bpf_prog *xdp_prog)\n> +{\n> +\tstruct redirect_info *ri = this_cpu_ptr(&redirect_info);\n> +\tstruct bpf_map *map = ri->map;\n> +\tu32 index = ri->ifindex;\n> +\tstruct net_device *fwd;\n> +\tint err;\n> +\n> +\tri->ifindex = 0;\n> +\tri->map = NULL;\n> +\n> +\tfwd = __dev_map_lookup_elem(map, index);\n> +\tif (!fwd) {\n> +\t\terr = -EINVAL;\n> +\t\tgoto err;\n> +\t}\n> +\tskb->dev = fwd;\n> +\t_trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);\n> +\treturn 0;\n> +err:\n> +\t_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);\n> +\treturn err;\n> +}\n> +\n> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n> \t\t\t struct bpf_prog *xdp_prog)\n> {\n> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n> \tunsigned int len;\n> \tint err = 0;\n>\n> +\tif (ri->map)\n> +\t\treturn xdp_do_generic_redirect_map(dev, skb, xdp_prog);\n\nThis is not quite correct. Really, the only thing you want\nto do here is more or less ...\n\nint xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n\t\t\t struct bpf_prog *xdp_prog)\n{\n\tstruct redirect_info *ri = this_cpu_ptr(&redirect_info);\n\tstruct bpf_map *map = ri->map;\n\tu32 index = ri->ifindex;\n\tstruct net_device *fwd;\n\tunsigned int len;\n\tint err = 0;\n\n\tri->ifindex = 0;\n\tri->map = NULL;\n\n\tif (map)\n\t\tfwd = __dev_map_lookup_elem(map, index);\n\telse\n\t\tfwd = dev_get_by_index_rcu(dev_net(dev), index);\n\tif (unlikely(!fwd)) {\n\t\terr = -EINVAL;\n\t\tgoto err;\n\t}\n[...]\n\n... such that you have a common path to also do the IFF_UP\nand MTU checks that are done here, but otherwise omitted in\nyour patch.\n\nOtherwise it looks good, but note that it also doesn't really\nresolve the issue you mention wrt stale map pointers by the\nway. This would need a different way to clear out the pointers\nfrom redirect_info, I'm thinking when we have devmap dismantle\ntime after RCU grace period we should check whether there are\nstill stale pointers from this map around and clear them under\ndisabled preemption, but need to brainstorm a bit more on that\nfirst.\n\n> \tfwd = dev_get_by_index_rcu(dev_net(dev), index);\n> \tri->ifindex = 0;\n> \tif (unlikely(!fwd)) {\n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnTRx27hqz9s7F\n\tfor ;\n\tThu, 7 Sep 2017 02:24:13 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755432AbdIFQYL (ORCPT );\n\tWed, 6 Sep 2017 12:24:11 -0400","from www62.your-server.de ([213.133.104.62]:59644 \"EHLO\n\twww62.your-server.de\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1754774AbdIFQYJ (ORCPT\n\t); Wed, 6 Sep 2017 12:24:09 -0400","from [92.105.166.74] (helo=localhost.localdomain)\n\tby www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-SHA:256)\n\t(Exim 4.85_2) (envelope-from )\n\tid 1dpd7X-0003vI-Mi; Wed, 06 Sep 2017 18:24:07 +0200"],"Message-ID":"<59B02127.5020904@iogearbox.net>","Date":"Wed, 06 Sep 2017 18:24:07 +0200","From":"Daniel Borkmann ","User-Agent":"Mozilla/5.0 (X11; Linux x86_64;\n\trv:31.0) Gecko/20100101 Thunderbird/31.7.0","MIME-Version":"1.0","To":"Jesper Dangaard Brouer , netdev@vger.kernel.org,\n\t\"David S. Miller\" ","CC":"John Fastabend ,\n\tAndy Gospodarek ","Subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","References":"<150471158528.3727.12324542627400287360.stgit@firesoul>","In-Reply-To":"<150471158528.3727.12324542627400287360.stgit@firesoul>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","X-Authenticated-Sender":"daniel@iogearbox.net","X-Virus-Scanned":"Clear (ClamAV 0.99.2/23782/Wed Sep 6 14:33:58 2017)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1764265,"web_url":"http://patchwork.ozlabs.org/comment/1764265/","msgid":"<59B02A39.4060603@iogearbox.net>","list_archive_url":null,"date":"2017-09-06T17:02:49","subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","submitter":{"id":65705,"url":"http://patchwork.ozlabs.org/api/people/65705/","name":"Daniel Borkmann","email":"daniel@iogearbox.net"},"content":"On 09/06/2017 06:24 PM, Daniel Borkmann wrote:\n[...]\n> Otherwise it looks good, but note that it also doesn't really\n> resolve the issue you mention wrt stale map pointers by the\n> way. This would need a different way to clear out the pointers\n> from redirect_info, I'm thinking when we have devmap dismantle\n> time after RCU grace period we should check whether there are\n> still stale pointers from this map around and clear them under\n> disabled preemption, but need to brainstorm a bit more on that\n> first.\n\nScratch that approach, doesn't work. So thinking bit more on\nthis, what we could do here is the following: verifier knows\nwe called bpf_xdp_redirect_map() helper, so it could do a small\ninsn rewrite in the sense that it fills R4 with a pointer to\nthe bpf_prog. We have that at verification time anyway and R4\nis allowed to be populated since we scratch it per convention.\nThen, the helper would store the prog pointer in struct redirect_info.\nLater in xdp_do_*_redirect() we check whether the redirect_info's\nprog pointer is the same as passed xdp_prog pointer, and if\nthat's the case then all good, since the prog holds a ref on\nthe map anyway, if they are not equal in the unlikely case, it\nmeans stale pointer, so we bail out right there. That would\nwork imo, will see to code it up and check it out.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnVJf6cD7z9t2d\n\tfor ;\n\tThu, 7 Sep 2017 03:02:58 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754551AbdIFRCy (ORCPT );\n\tWed, 6 Sep 2017 13:02:54 -0400","from www62.your-server.de ([213.133.104.62]:36829 \"EHLO\n\twww62.your-server.de\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1754364AbdIFRCx (ORCPT\n\t); Wed, 6 Sep 2017 13:02:53 -0400","from [92.105.166.74] (helo=localhost.localdomain)\n\tby www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-SHA:256)\n\t(Exim 4.85_2) (envelope-from )\n\tid 1dpdj0-00060v-03; Wed, 06 Sep 2017 19:02:50 +0200"],"Message-ID":"<59B02A39.4060603@iogearbox.net>","Date":"Wed, 06 Sep 2017 19:02:49 +0200","From":"Daniel Borkmann ","User-Agent":"Mozilla/5.0 (X11; Linux x86_64;\n\trv:31.0) Gecko/20100101 Thunderbird/31.7.0","MIME-Version":"1.0","To":"Jesper Dangaard Brouer , netdev@vger.kernel.org,\n\t\"David S. Miller\" ","CC":"John Fastabend ,\n\tAndy Gospodarek ","Subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","References":"<150471158528.3727.12324542627400287360.stgit@firesoul>\n\t<59B02127.5020904@iogearbox.net>","In-Reply-To":"<59B02127.5020904@iogearbox.net>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","X-Authenticated-Sender":"daniel@iogearbox.net","X-Virus-Scanned":"Clear (ClamAV 0.99.2/23782/Wed Sep 6 14:33:58 2017)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1764304,"web_url":"http://patchwork.ozlabs.org/comment/1764304/","msgid":"<20170906201835.7bc26f4f@redhat.com>","list_archive_url":null,"date":"2017-09-06T18:18:35","subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","submitter":{"id":13625,"url":"http://patchwork.ozlabs.org/api/people/13625/","name":"Jesper Dangaard Brouer","email":"brouer@redhat.com"},"content":"On Wed, 06 Sep 2017 18:24:07 +0200\nDaniel Borkmann wrote:\n\n> On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:\n> > Using bpf_redirect_map is allowed for generic XDP programs, but the\n> > appropriate map lookup was never performed in xdp_do_generic_redirect().\n> >\n> > Instead the map-index is directly used as the ifindex. For the \n> \n> Good point, but ...\n> \n> [...]\n> > net/core/filter.c | 29 +++++++++++++++++++++++++++++\n> > 1 file changed, 29 insertions(+)\n> >\n> > diff --git a/net/core/filter.c b/net/core/filter.c\n> > index 5912c738a7b2..6a4745bf2c9f 100644\n> > --- a/net/core/filter.c\n> > +++ b/net/core/filter.c\n> > @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,\n> > }\n> > EXPORT_SYMBOL_GPL(xdp_do_redirect);\n> >\n> > +static int xdp_do_generic_redirect_map(struct net_device *dev,\n> > +\t\t\t\t struct sk_buff *skb,\n> > +\t\t\t\t struct bpf_prog *xdp_prog)\n> > +{\n> > +\tstruct redirect_info *ri = this_cpu_ptr(&redirect_info);\n> > +\tstruct bpf_map *map = ri->map;\n> > +\tu32 index = ri->ifindex;\n> > +\tstruct net_device *fwd;\n> > +\tint err;\n> > +\n> > +\tri->ifindex = 0;\n> > +\tri->map = NULL;\n> > +\n> > +\tfwd = __dev_map_lookup_elem(map, index);\n> > +\tif (!fwd) {\n> > +\t\terr = -EINVAL;\n> > +\t\tgoto err;\n> > +\t}\n> > +\tskb->dev = fwd;\n> > +\t_trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);\n> > +\treturn 0;\n> > +err:\n> > +\t_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);\n> > +\treturn err;\n> > +}\n> > +\n> > int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n> > \t\t\t struct bpf_prog *xdp_prog)\n> > {\n> > @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n> > \tunsigned int len;\n> > \tint err = 0;\n> >\n> > +\tif (ri->map)\n> > +\t\treturn xdp_do_generic_redirect_map(dev, skb, xdp_prog); \n> \n> This is not quite correct. Really, the only thing you want\n> to do here is more or less ...\n> \n> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n> \t\t\t struct bpf_prog *xdp_prog)\n> {\n> \tstruct redirect_info *ri = this_cpu_ptr(&redirect_info);\n> \tstruct bpf_map *map = ri->map;\n> \tu32 index = ri->ifindex;\n> \tstruct net_device *fwd;\n> \tunsigned int len;\n> \tint err = 0;\n> \n> \tri->ifindex = 0;\n> \tri->map = NULL;\n> \n> \tif (map)\n> \t\tfwd = __dev_map_lookup_elem(map, index);\n> \telse\n> \t\tfwd = dev_get_by_index_rcu(dev_net(dev), index);\n> \tif (unlikely(!fwd)) {\n> \t\terr = -EINVAL;\n> \t\tgoto err;\n> \t}\n> [...]\n> \n> ... such that you have a common path to also do the IFF_UP\n> and MTU checks that are done here, but otherwise omitted in\n> your patch.\n\nAh, yes. My patch miss the IFF_UP and MTU check. (I was too inspired by\nxdp_do_redirect_map).\n\n\n> Otherwise it looks good, but note that it also doesn't really\n> resolve the issue you mention wrt stale map pointers by the\n> way. [...]\n\nI actually discovered more cases where we can crash the kernel :-(\n\nE.g. driver not supporting XDP_REDIRECT, are still allowed to load an\nXDP bpf_prog that call bpf_redirect_map() and set the ->map pointer,\nbut it will never call xdp_do_redirect() (which is responsible for\nclearing/consuming ->map pointer).\n\nAnother case: You can also call bpf_redirect_map() and then NOT return\nXDP_REDIRECT (it is obviously strange, but the bpf-helper API allows it).","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=brouer@redhat.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnX063bH1z9sRY\n\tfor ;\n\tThu, 7 Sep 2017 04:18:46 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751369AbdIFSSn (ORCPT );\n\tWed, 6 Sep 2017 14:18:43 -0400","from mx1.redhat.com ([209.132.183.28]:47180 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1750836AbdIFSSm (ORCPT );\n\tWed, 6 Sep 2017 14:18:42 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 9C5F25F7AE;\n\tWed, 6 Sep 2017 18:18:42 +0000 (UTC)","from localhost (ovpn-200-27.brq.redhat.com [10.40.200.27])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 170A7179D1;\n\tWed, 6 Sep 2017 18:18:36 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 9C5F25F7AE","Date":"Wed, 6 Sep 2017 20:18:35 +0200","From":"Jesper Dangaard Brouer ","To":"Daniel Borkmann ","Cc":"netdev@vger.kernel.org, \"David S. Miller\" ,\n\tJohn Fastabend ,\n\tAndy Gospodarek , brouer@redhat.com","Subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","Message-ID":"<20170906201835.7bc26f4f@redhat.com>","In-Reply-To":"<59B02127.5020904@iogearbox.net>","References":"<150471158528.3727.12324542627400287360.stgit@firesoul>\n\t<59B02127.5020904@iogearbox.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tWed, 06 Sep 2017 18:18:42 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1764309,"web_url":"http://patchwork.ozlabs.org/comment/1764309/","msgid":"<59B0418A.5090609@gmail.com>","list_archive_url":null,"date":"2017-09-06T18:42:18","subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","submitter":{"id":20028,"url":"http://patchwork.ozlabs.org/api/people/20028/","name":"John Fastabend","email":"john.fastabend@gmail.com"},"content":"On 09/06/2017 11:18 AM, Jesper Dangaard Brouer wrote:\n> On Wed, 06 Sep 2017 18:24:07 +0200\n> Daniel Borkmann wrote:\n> \n>> On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:\n>>> Using bpf_redirect_map is allowed for generic XDP programs, but the\n>>> appropriate map lookup was never performed in xdp_do_generic_redirect().\n>>>\n>>> Instead the map-index is directly used as the ifindex. For the \n>>\n>> Good point, but ...\n>>\n>> [...]\n>>> net/core/filter.c | 29 +++++++++++++++++++++++++++++\n>>> 1 file changed, 29 insertions(+)\n>>>\n>>> diff --git a/net/core/filter.c b/net/core/filter.c\n>>> index 5912c738a7b2..6a4745bf2c9f 100644\n>>> --- a/net/core/filter.c\n>>> +++ b/net/core/filter.c\n>>> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,\n>>> }\n>>> EXPORT_SYMBOL_GPL(xdp_do_redirect);\n>>>\n>>> +static int xdp_do_generic_redirect_map(struct net_device *dev,\n>>> +\t\t\t\t struct sk_buff *skb,\n>>> +\t\t\t\t struct bpf_prog *xdp_prog)\n>>> +{\n>>> +\tstruct redirect_info *ri = this_cpu_ptr(&redirect_info);\n>>> +\tstruct bpf_map *map = ri->map;\n>>> +\tu32 index = ri->ifindex;\n>>> +\tstruct net_device *fwd;\n>>> +\tint err;\n>>> +\n>>> +\tri->ifindex = 0;\n>>> +\tri->map = NULL;\n>>> +\n>>> +\tfwd = __dev_map_lookup_elem(map, index);\n>>> +\tif (!fwd) {\n>>> +\t\terr = -EINVAL;\n>>> +\t\tgoto err;\n>>> +\t}\n>>> +\tskb->dev = fwd;\n>>> +\t_trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);\n>>> +\treturn 0;\n>>> +err:\n>>> +\t_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);\n>>> +\treturn err;\n>>> +}\n>>> +\n>>> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n>>> \t\t\t struct bpf_prog *xdp_prog)\n>>> {\n>>> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n>>> \tunsigned int len;\n>>> \tint err = 0;\n>>>\n>>> +\tif (ri->map)\n>>> +\t\treturn xdp_do_generic_redirect_map(dev, skb, xdp_prog); \n>>\n>> This is not quite correct. Really, the only thing you want\n>> to do here is more or less ...\n>>\n>> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n>> \t\t\t struct bpf_prog *xdp_prog)\n>> {\n>> \tstruct redirect_info *ri = this_cpu_ptr(&redirect_info);\n>> \tstruct bpf_map *map = ri->map;\n>> \tu32 index = ri->ifindex;\n>> \tstruct net_device *fwd;\n>> \tunsigned int len;\n>> \tint err = 0;\n>>\n>> \tri->ifindex = 0;\n>> \tri->map = NULL;\n>>\n>> \tif (map)\n>> \t\tfwd = __dev_map_lookup_elem(map, index);\n>> \telse\n>> \t\tfwd = dev_get_by_index_rcu(dev_net(dev), index);\n>> \tif (unlikely(!fwd)) {\n>> \t\terr = -EINVAL;\n>> \t\tgoto err;\n>> \t}\n>> [...]\n>>\n>> ... such that you have a common path to also do the IFF_UP\n>> and MTU checks that are done here, but otherwise omitted in\n>> your patch.\n> \n> Ah, yes. My patch miss the IFF_UP and MTU check. (I was too inspired by\n> xdp_do_redirect_map).\n> \n> \n>> Otherwise it looks good, but note that it also doesn't really\n>> resolve the issue you mention wrt stale map pointers by the\n>> way. [...]\n> \n> I actually discovered more cases where we can crash the kernel :-(\n> \n> E.g. driver not supporting XDP_REDIRECT, are still allowed to load an\n> XDP bpf_prog that call bpf_redirect_map() and set the ->map pointer,\n> but it will never call xdp_do_redirect() (which is responsible for\n> clearing/consuming ->map pointer).\n> \n> Another case: You can also call bpf_redirect_map() and then NOT return\n> XDP_REDIRECT (it is obviously strange, but the bpf-helper API allows it).\n> \n\nI think we can cover both these cases with previous suggestion to check\nprog pointers. Working up a patch now.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"e38dirHv\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnXWX6TMvz9sRY\n\tfor ;\n\tThu, 7 Sep 2017 04:42:32 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752232AbdIFSm3 (ORCPT );\n\tWed, 6 Sep 2017 14:42:29 -0400","from mail-pg0-f65.google.com ([74.125.83.65]:34909 \"EHLO\n\tmail-pg0-f65.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752142AbdIFSm2 (ORCPT\n\t); Wed, 6 Sep 2017 14:42:28 -0400","by mail-pg0-f65.google.com with SMTP id q68so3865666pgq.2\n\tfor ; Wed, 06 Sep 2017 11:42:28 -0700 (PDT)","from [192.168.86.74] ([72.168.144.71])\n\tby smtp.gmail.com with ESMTPSA id\n\t62sm689024pfs.33.2017.09.06.11.42.22\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tWed, 06 Sep 2017 11:42:27 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=message-id:date:from:user-agent:mime-version:to:cc:subject\n\t:references:in-reply-to:content-transfer-encoding;\n\tbh=lGRPc368DlnnGYreC157xEasF1slcxS2zA7E1b5GZQo=;\n\tb=e38dirHv9Lu9FTolR/8OHDGES7fHQDt7npQT/Issvl6LPiCIuDDmN4lJ80gWJUUr1w\n\tbaznavljmxnTd8Kx2EYOxHYhBiejQ2McCLzzD5LW5HhAi6vE6dXG2ny/Y0DHFj4IZLES\n\tzjCFMBCyGzqKsyNnj5yp8K2/USS9HTKTNCG7QdP2mts2pYwAAMSF/wIKfcuGemXg65pq\n\tDrPRC2d3hx36l5VNyIIKPNZkDNiUlYn+UiGLtIzDQPBqsJa296StQi2v3Nn9Vjlle2Ma\n\tDFAeIIomu5leh8Mp7LG7Grq3FE7Fo2umyvnvZ51/VCgH6Uus94ENs/vYFKyL/XDZtmYx\n\tHVIw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:date:from:user-agent:mime-version:to\n\t:cc:subject:references:in-reply-to:content-transfer-encoding;\n\tbh=lGRPc368DlnnGYreC157xEasF1slcxS2zA7E1b5GZQo=;\n\tb=ahBmTa5GOM6tWYNZne7ZYTa+6ow39rwnEJ1JLvpqy6KvvMoJ+8EcR3Llf+9JNya18q\n\tH0HHhZZYPq4dUdkyUaE/Erz+FQxzvz8ab0/CGGs7CWI3Zuj68otisqOZMVp6Na1NtAwP\n\t0KyDrwLGlDar6rU+9rlY323m8te4DLhR9i45UQZdDxkzAW4cNBJiu9hxg5b9uRHLWVLp\n\tto/yPVRtENMdPtKXUuDRP8CLiUsDQ7jAdqzTi6shfuQK1o50wabY0JBRhB3AF8d4mhcC\n\taD0ytWgMdDvy6yEc5uRYoOMKxHSLph2pojXmdNAq70Sb1MNuqTmaIgc2SVH2jJhkjZBe\n\thqmQ==","X-Gm-Message-State":"AHPjjUiFaxhV4RKqu/U80pnt15UGfM3hpwk8zEth3Pw+AvOqCh3AVjzY\n\tTqXdKiyvEF2IZbOy","X-Google-Smtp-Source":"ADKCNb6gvkup56O+wYrOc/q8v0m7leudQa8E1JlOlG7I1OKoDSXjDvWR1MleWtEjRjuqqZpzcR427w==","X-Received":"by 10.159.218.8 with SMTP id v8mr142983plp.46.1504723348244;\n\tWed, 06 Sep 2017 11:42:28 -0700 (PDT)","Message-ID":"<59B0418A.5090609@gmail.com>","Date":"Wed, 06 Sep 2017 11:42:18 -0700","From":"John Fastabend ","User-Agent":"Mozilla/5.0 (X11; Linux x86_64;\n\trv:31.0) Gecko/20100101 Thunderbird/31.6.0","MIME-Version":"1.0","To":"Jesper Dangaard Brouer ,\n\tDaniel Borkmann ","CC":"netdev@vger.kernel.org, \"David S. Miller\" ,\n\tAndy Gospodarek ","Subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","References":"<150471158528.3727.12324542627400287360.stgit@firesoul>\n\t<59B02127.5020904@iogearbox.net>\n\t<20170906201835.7bc26f4f@redhat.com>","In-Reply-To":"<20170906201835.7bc26f4f@redhat.com>","Content-Type":"text/plain; charset=windows-1252","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1764310,"web_url":"http://patchwork.ozlabs.org/comment/1764310/","msgid":"<59B043BA.1030001@iogearbox.net>","list_archive_url":null,"date":"2017-09-06T18:51:38","subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","submitter":{"id":65705,"url":"http://patchwork.ozlabs.org/api/people/65705/","name":"Daniel Borkmann","email":"daniel@iogearbox.net"},"content":"On 09/06/2017 08:42 PM, John Fastabend wrote:\n> On 09/06/2017 11:18 AM, Jesper Dangaard Brouer wrote:\n>> On Wed, 06 Sep 2017 18:24:07 +0200\n>> Daniel Borkmann wrote:\n>>> On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:\n>>>> Using bpf_redirect_map is allowed for generic XDP programs, but the\n>>>> appropriate map lookup was never performed in xdp_do_generic_redirect().\n>>>>\n>>>> Instead the map-index is directly used as the ifindex. For the\n>>>\n>>> Good point, but ...\n>>>\n>>> [...]\n>>>> net/core/filter.c | 29 +++++++++++++++++++++++++++++\n>>>> 1 file changed, 29 insertions(+)\n>>>>\n>>>> diff --git a/net/core/filter.c b/net/core/filter.c\n>>>> index 5912c738a7b2..6a4745bf2c9f 100644\n>>>> --- a/net/core/filter.c\n>>>> +++ b/net/core/filter.c\n>>>> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,\n>>>> }\n>>>> EXPORT_SYMBOL_GPL(xdp_do_redirect);\n>>>>\n>>>> +static int xdp_do_generic_redirect_map(struct net_device *dev,\n>>>> +\t\t\t\t struct sk_buff *skb,\n>>>> +\t\t\t\t struct bpf_prog *xdp_prog)\n>>>> +{\n>>>> +\tstruct redirect_info *ri = this_cpu_ptr(&redirect_info);\n>>>> +\tstruct bpf_map *map = ri->map;\n>>>> +\tu32 index = ri->ifindex;\n>>>> +\tstruct net_device *fwd;\n>>>> +\tint err;\n>>>> +\n>>>> +\tri->ifindex = 0;\n>>>> +\tri->map = NULL;\n>>>> +\n>>>> +\tfwd = __dev_map_lookup_elem(map, index);\n>>>> +\tif (!fwd) {\n>>>> +\t\terr = -EINVAL;\n>>>> +\t\tgoto err;\n>>>> +\t}\n>>>> +\tskb->dev = fwd;\n>>>> +\t_trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);\n>>>> +\treturn 0;\n>>>> +err:\n>>>> +\t_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);\n>>>> +\treturn err;\n>>>> +}\n>>>> +\n>>>> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n>>>> \t\t\t struct bpf_prog *xdp_prog)\n>>>> {\n>>>> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n>>>> \tunsigned int len;\n>>>> \tint err = 0;\n>>>>\n>>>> +\tif (ri->map)\n>>>> +\t\treturn xdp_do_generic_redirect_map(dev, skb, xdp_prog);\n>>>\n>>> This is not quite correct. Really, the only thing you want\n>>> to do here is more or less ...\n>>>\n>>> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,\n>>> \t\t\t struct bpf_prog *xdp_prog)\n>>> {\n>>> \tstruct redirect_info *ri = this_cpu_ptr(&redirect_info);\n>>> \tstruct bpf_map *map = ri->map;\n>>> \tu32 index = ri->ifindex;\n>>> \tstruct net_device *fwd;\n>>> \tunsigned int len;\n>>> \tint err = 0;\n>>>\n>>> \tri->ifindex = 0;\n>>> \tri->map = NULL;\n>>>\n>>> \tif (map)\n>>> \t\tfwd = __dev_map_lookup_elem(map, index);\n>>> \telse\n>>> \t\tfwd = dev_get_by_index_rcu(dev_net(dev), index);\n>>> \tif (unlikely(!fwd)) {\n>>> \t\terr = -EINVAL;\n>>> \t\tgoto err;\n>>> \t}\n>>> [...]\n>>>\n>>> ... such that you have a common path to also do the IFF_UP\n>>> and MTU checks that are done here, but otherwise omitted in\n>>> your patch.\n>>\n>> Ah, yes. My patch miss the IFF_UP and MTU check. (I was too inspired by\n>> xdp_do_redirect_map).\n>>\n>>> Otherwise it looks good, but note that it also doesn't really\n>>> resolve the issue you mention wrt stale map pointers by the\n>>> way. [...]\n>>\n>> I actually discovered more cases where we can crash the kernel :-(\n>>\n>> E.g. driver not supporting XDP_REDIRECT, are still allowed to load an\n>> XDP bpf_prog that call bpf_redirect_map() and set the ->map pointer,\n>> but it will never call xdp_do_redirect() (which is responsible for\n>> clearing/consuming ->map pointer).\n>>\n>> Another case: You can also call bpf_redirect_map() and then NOT return\n>> XDP_REDIRECT (it is obviously strange, but the bpf-helper API allows it).\n>\n> I think we can cover both these cases with previous suggestion to check\n> prog pointers. Working up a patch now.\n\nYep, they would both be covered.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnXkF2fHKz9sRV\n\tfor ;\n\tThu, 7 Sep 2017 04:51:49 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751973AbdIFSvq (ORCPT );\n\tWed, 6 Sep 2017 14:51:46 -0400","from www62.your-server.de ([213.133.104.62]:53591 \"EHLO\n\twww62.your-server.de\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751345AbdIFSvq (ORCPT\n\t); Wed, 6 Sep 2017 14:51:46 -0400","from [92.105.166.74] (helo=localhost.localdomain)\n\tby www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-SHA:256)\n\t(Exim 4.85_2) (envelope-from )\n\tid 1dpfQJ-0007F7-4o; Wed, 06 Sep 2017 20:51:39 +0200"],"Message-ID":"<59B043BA.1030001@iogearbox.net>","Date":"Wed, 06 Sep 2017 20:51:38 +0200","From":"Daniel Borkmann ","User-Agent":"Mozilla/5.0 (X11; Linux x86_64;\n\trv:31.0) Gecko/20100101 Thunderbird/31.7.0","MIME-Version":"1.0","To":"John Fastabend ,\n\tJesper Dangaard Brouer ","CC":"netdev@vger.kernel.org, \"David S. Miller\" ,\n\tAndy Gospodarek ","Subject":"Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP","References":"<150471158528.3727.12324542627400287360.stgit@firesoul>\n\t<59B02127.5020904@iogearbox.net>\n\t<20170906201835.7bc26f4f@redhat.com> <59B0418A.5090609@gmail.com>","In-Reply-To":"<59B0418A.5090609@gmail.com>","Content-Type":"text/plain; charset=windows-1252; format=flowed","Content-Transfer-Encoding":"7bit","X-Authenticated-Sender":"daniel@iogearbox.net","X-Virus-Scanned":"Clear (ClamAV 0.99.2/23783/Wed Sep 6 18:36:21 2017)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]