* Re: [PATCH] net: stmmac: add sanity check to device_property_read_u32_array call
From: Colin Ian King @ 2019-06-28 8:32 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: alexandre.torgue, davem, joabreu, kernel-janitors,
linux-arm-kernel, linux-kernel, linux-stm32, mcoquelin.stm32,
netdev, peppe.cavallaro
In-Reply-To: <CAFBinCBk5aPVE+vq5px3QKS1T_R=WGXXxEJMC9X676KGvi9jdg@mail.gmail.com>
On 28/06/2019 05:15, Martin Blumenstingl wrote:
> On Tue, Jun 25, 2019 at 9:58 AM Colin Ian King <colin.king@canonical.com> wrote:
>>
>> On 25/06/2019 05:44, Martin Blumenstingl wrote:
>>> Hi Colin,
>>>
>>> On Thu, Jun 20, 2019 at 3:34 AM Martin Blumenstingl
>>> <martin.blumenstingl@googlemail.com> wrote:
>>>>
>>>> Hi Colin,
>>>>
>>>> On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@canonical.com> wrote:
>>>>>
>>>>> On 19/06/2019 06:13, Martin Blumenstingl wrote:
>>>>>> Hi Colin,
>>>>>>
>>>>>>> Currently the call to device_property_read_u32_array is not error checked
>>>>>>> leading to potential garbage values in the delays array that are then used
>>>>>>> in msleep delays. Add a sanity check to the property fetching.
>>>>>>>
>>>>>>> Addresses-Coverity: ("Uninitialized scalar variable")
>>>>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>>>> I have also sent a patch [0] to fix initialize the array.
>>>>>> can you please look at my patch so we can work out which one to use?
>>>>>>
>>>>>> my concern is that the "snps,reset-delays-us" property is optional,
>>>>>> the current dt-bindings documentation states that it's a required
>>>>>> property. in reality it isn't, there are boards (two examples are
>>>>>> mentioned in my patch: [0]) without it.
>>>>>>
>>>>>> so I believe that the resulting behavior has to be:
>>>>>> 1. don't delay if this property is missing (instead of delaying for
>>>>>> <garbage value> ms)
>>>>>> 2. don't error out if this property is missing
>>>>>>
>>>>>> your patch covers #1, can you please check whether #2 is also covered?
>>>>>> I tested case #2 when submitting my patch and it worked fine (even
>>>>>> though I could not reproduce the garbage values which are being read
>>>>>> on some boards)
>>> in the meantime I have tested your patch.
>>> when I don't set the "snps,reset-delays-us" property then I get the
>>> following error:
>>> invalid property snps,reset-delays-us
>>>
>>> my patch has landed in the meantime: [0]
>>> how should we proceed with your patch?
Your fix is good, so I think we should just drop/forget about my fix.
Colin
>>
>> I'm out of the office today. I'll get back to you on this tomorrow.
> gentle ping
> (I will be away for the weekend but I can reply on Monday)
>
^ permalink raw reply
* Re: XDP multi-buffer incl. jumbo-frames (Was: [RFC V1 net-next 1/1] net: ena: implement XDP drop support)
From: Jesper Dangaard Brouer @ 2019-06-28 8:46 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Toke Høiland-Jørgensen, Machulsky, Zorik, Jubran, Samih,
davem@davemloft.net, netdev@vger.kernel.org, Woodhouse, David,
Matushevsky, Alexander, Bshara, Saeed, Wilson, Matt,
Liguori, Anthony, Bshara, Nafea, Tzalik, Guy, Belgazal, Netanel,
Saidi, Ali, Herrenschmidt, Benjamin, Kiyanovski, Arthur,
Daniel Borkmann, Ilias Apalodimas, Alexei Starovoitov,
Jakub Kicinski, xdp-newbies@vger.kernel.org, brouer
In-Reply-To: <CA+FuTSfKnhv9rr=cDa_4m7Dd9qkEm_oabDfyvH0T0sM+fQTU=w@mail.gmail.com>
On Wed, 26 Jun 2019 11:20:45 -0400 Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> On Wed, Jun 26, 2019 at 11:01 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> > Jesper Dangaard Brouer <brouer@redhat.com> writes:
> > > On Wed, 26 Jun 2019 13:52:16 +0200
> > > Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> > >
> > >> Jesper Dangaard Brouer <brouer@redhat.com> writes:
> > >>
[...]
> > >
> > > You touch upon some interesting complications already:
> > >
> > > 1. It is valuable for XDP bpf_prog to know "full" length?
> > > (if so, then we need to extend xdp ctx with info)
> >
> > Valuable, quite likely. A hard requirement, probably not (for all use
> > cases).
>
> Agreed.
>
> One common validation use would be to drop any packets whose header
> length disagrees with the actual packet length.
That is a good point.
Added a section "XDP access to full packet length?" to capture this:
- https://github.com/xdp-project/xdp-project/commit/da5b84264b85b0d
- https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org#xdp-access-to-full-packet-length
> > > But if we need to know the full length, when the first-buffer is
> > > processed. Then realize that this affect the drivers RX-loop, because
> > > then we need to "collect" all the buffers before we can know the
> > > length (although some HW provide this in first descriptor).
> > >
> > > We likely have to change drivers RX-loop anyhow, as XDP_TX and
> > > XDP_REDIRECT will also need to "collect" all buffers before the packet
> > > can be forwarded. (Although this could potentially happen later in
> > > driver loop when it meet/find the End-Of-Packet descriptor bit).
>
> Yes, this might be quite a bit of refactoring of device driver code.
>
> Should we move forward with some initial constraints, e.g., no
> XDP_REDIRECT, no "full" length and no bpf_xdp_adjust_tail?
I generally like this...
If not adding "full" length. Maybe we could add an indication to
XDP-developer, that his is a multi-buffer/multi-segment packet, such
that header length validation code against packet length (data_end-data)
is not possible. This is user visible, so we would have to keep it
forever... I'm leaning towards adding "full" length from beginning.
> That already allows many useful programs.
>
> As long as we don't arrive at a design that cannot be extended with
> those features later.
That is the important part...
> > > 2. Can we even allow helper bpf_xdp_adjust_tail() ?
[...]
> >
> > > Perhaps it is better to let bpf_xdp_adjust_tail() fail runtime?
> >
> > If we do disallow it, I think I'd lean towards failing the call at
> > runtime...
>
> Disagree. I'd rather have a program fail at load if it depends on
> multi-frag support while the (driver) implementation does not yet
> support it.
I usually agree that we should fail the program, early at load time.
For XDP we are unfortunately missing some knobs to do this, see[1].
Specifically for bpf_xdp_adjust_tail(), it might be better to fail
runtime. Because, the driver might have enabled TSO for TCP packets,
while your XDP use-case is for adjusting UDP-packets (and do XDP level
replies), which will never see multi-buffer packets. If we fail use of
bpf_xdp_adjust_tail(), then you would have to disable TSO to allow
loading your XDP-prog, hurting the other TSO-TCP use-case.
[1] http://vger.kernel.org/netconf2019_files/xdp-feature-detection.pdf
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next 10/16] qlge: Factor out duplicated expression
From: Benjamin Poirier @ 2019-06-28 8:52 UTC (permalink / raw)
To: Manish Chopra; +Cc: David Miller, GR-Linux-NIC-Dev, netdev@vger.kernel.org
In-Reply-To: <DM6PR18MB26974EE53AF9BC66D9E3F0FEABE30@DM6PR18MB2697.namprd18.prod.outlook.com>
On 2019/06/25 18:32, Manish Chopra wrote:
[...]
> >
> > What I inferred from the presence of that expression though is that in the
> > places where it is used, the device interprets a value of 0 as 65536. Manish,
> > can you confirm that? As David points out, the expression is useless. A
> > comment might not be however.
>
> Yes, I think it could be simplified to simple cast just.
>
I checked and it seems like the device treats cqicq->lbq_buf_size = 0
more like 65536 than like 0. That is, I saw it write up to 9596B
(running at 9000 mtu...) in a single lbq buffer.
^ permalink raw reply
* Re: [PATCH v4 net-next 1/4] net: core: page_pool: add user cnt preventing pool deletion
From: Ivan Khoronzhuk @ 2019-06-28 8:53 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: davem, grygorii.strashko, saeedm, leon, ast, linux-kernel,
linux-omap, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend
In-Reply-To: <20190628083520.4203cb41@carbon>
On Fri, Jun 28, 2019 at 08:35:20AM +0200, Jesper Dangaard Brouer wrote:
>On Fri, 28 Jun 2019 01:02:47 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> Hi Jesper, thanks you remember about it.
>>
>> >
>> >I don't think that "create" and "free" routines paring looks "more
>> >correct" together.
>> >
>> >Maybe we can scale back your solution(?), via creating a page_pool_get()
>> >and page_pool_put() API that can be used by your driver, to keep the
>> >page_pool object after a xdp_rxq_info_unreg() call. Then you can use
>> >it for two xdp_rxq_info structs, and call page_pool_put() after you
>> >have unregistered both.
>> >
>> >The API would basically be:
>> >
>> >diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>> >index b366f59885c1..691ddacfb5a6 100644
>> >--- a/net/core/page_pool.c
>> >+++ b/net/core/page_pool.c
>> >@@ -357,6 +357,10 @@ static void __warn_in_flight(struct page_pool *pool)
>> > void __page_pool_free(struct page_pool *pool)
>> > {
>> > WARN(pool->alloc.count, "API usage violation");
>> >+
>> >+ if (atomic_read(&pool->user_cnt) != 0)
>> >+ return;
>> >+
>> > WARN(!ptr_ring_empty(&pool->ring), "ptr_ring is not empty");
>> >
>> > /* Can happen due to forced shutdown */
>> >@@ -372,6 +376,19 @@ void __page_pool_free(struct page_pool *pool)
>> > }
>> > EXPORT_SYMBOL(__page_pool_free);
>> >
>> >+void page_pool_put(struct page_pool *pool)
>> >+{
>> >+ if (!atomic_dec_and_test(&pool->user_cnt))
>> >+ __page_pool_free(pool);
>> >+}
>> >+EXPORT_SYMBOL(page_pool_put);
>> >+
>> >+void page_pool_get(struct page_pool *pool)
>> >+{
>> >+ atomic_inc(&pool->user_cnt);
>> >+}
>> >+EXPORT_SYMBOL(page_pool_get);
>> >+
>>
>> I have another solution that doesn't touch page pool and adds modifications
>> to xdp allocator. As for me it looks better and work wider, I don't need to
>> think about this in the driver also.
>>
>> It's supposed allocator works as before, no any changes to mlx5 and
>> page_pool API and its usage and seems like fits your requirements.
>> It still supposes that allocator runs under same napi softirq but allows
>> to reuse allocator.
>>
>> I have not verified yet, but looks like:
>>
>> diff --git a/include/net/xdp_priv.h b/include/net/xdp_priv.h
>> index 6a8cba6ea79a..995b21da2f27 100644
>> --- a/include/net/xdp_priv.h
>> +++ b/include/net/xdp_priv.h
>> @@ -18,6 +18,7 @@ struct xdp_mem_allocator {
>> struct rcu_head rcu;
>> struct delayed_work defer_wq;
>> unsigned long defer_warn;
>> + unsigned long refcnt;
>> };
>>
>> #endif /* __LINUX_NET_XDP_PRIV_H__ */
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index f98ab6b98674..6239483e3793 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -98,6 +98,12 @@ static bool __mem_id_disconnect(int id, bool force)
>> WARN(1, "Request remove non-existing id(%d), driver bug?", id);
>> return true;
>> }
>> +
>> + if (--xa->refcnt) {
>> + mutex_unlock(&mem_id_lock);
>> + return true;
>
>This doesn't work. This function __mem_id_disconnect() can be called
>multiple times. E.g. if there are in-flight packets.
Yes, it was draft. I still have not completely verify it due
to several changes in cpsw and holiday in my side, second draft
looks like:
Subject: [PATCH] net: core: xdp: allow same allocator for rxq
XDP rxq can be same for ndevs running under same rx napi softirq.
But there is no ability to register same allocator for both rxqs,
by fact it's same rxq but has different ndev as a reference.
Due to last changes allocator destroy can be defered till the moment
all packets are recycled by destination interface, afterwards it's
freed. In order to schedule allocator destroy only after all users are
unregistered, add refcnt to allocator object and start to destroy
only it reaches 0.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
include/net/xdp_priv.h | 1 +
net/core/xdp.c | 46 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/include/net/xdp_priv.h b/include/net/xdp_priv.h
index 6a8cba6ea79a..995b21da2f27 100644
--- a/include/net/xdp_priv.h
+++ b/include/net/xdp_priv.h
@@ -18,6 +18,7 @@ struct xdp_mem_allocator {
struct rcu_head rcu;
struct delayed_work defer_wq;
unsigned long defer_warn;
+ unsigned long refcnt;
};
#endif /* __LINUX_NET_XDP_PRIV_H__ */
diff --git a/net/core/xdp.c b/net/core/xdp.c
index f98ab6b98674..7b0185eec124 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -98,6 +98,18 @@ static bool __mem_id_disconnect(int id, bool force)
WARN(1, "Request remove non-existing id(%d), driver bug?", id);
return true;
}
+
+ /* to avoid hash lookup twice do refcnt dec here, but not when
+ * it's 0 as it can be called from workqueue aftewards
+ */
+ if (xa->refcnt)
+ xa->refcnt--;
+
+ if (xa->refcnt) {
+ mutex_unlock(&mem_id_lock);
+ return true;
+ }
+
xa->disconnect_cnt++;
/* Detects in-flight packet-pages for page_pool */
@@ -312,6 +324,33 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
return true;
}
+static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
+{
+ struct xdp_mem_allocator *xae, *xa = NULL;
+ struct rhashtable_iter iter;
+
+ mutex_lock(&mem_id_lock);
+ rhashtable_walk_enter(mem_id_ht, &iter);
+ do {
+ rhashtable_walk_start(&iter);
+
+ while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
+ if (xae->allocator == allocator) {
+ xae->refcnt++;
+ xa = xae;
+ break;
+ }
+ }
+
+ rhashtable_walk_stop(&iter);
+
+ } while (xae == ERR_PTR(-EAGAIN));
+ rhashtable_walk_exit(&iter);
+ mutex_unlock(&mem_id_lock);
+
+ return xa;
+}
+
int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
enum xdp_mem_type type, void *allocator)
{
@@ -347,6 +386,12 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
}
}
+ xdp_alloc = xdp_allocator_get(allocator);
+ if (xdp_alloc) {
+ xdp_rxq->mem.id = xdp_alloc->mem.id;
+ return 0;
+ }
+
xdp_alloc = kzalloc(sizeof(*xdp_alloc), gfp);
if (!xdp_alloc)
return -ENOMEM;
@@ -360,6 +405,7 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
xdp_rxq->mem.id = id;
xdp_alloc->mem = xdp_rxq->mem;
xdp_alloc->allocator = allocator;
+ xdp_alloc->refcnt = 1;
/* Insert allocator into ID lookup table */
ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply related
* Re: [PATCH iproute2-next v3 1/2] ipaddress: correctly print a VF hw address in the IPoIB case
From: Denis Kirjanov @ 2019-06-28 8:56 UTC (permalink / raw)
To: David Ahern; +Cc: stephen, netdev, linux-rdma, dledford, mkubecek
In-Reply-To: <a669939c-d8f3-f3c8-15f4-efa236e00954@gmail.com>
On 6/24/19, David Ahern <dsahern@gmail.com> wrote:
> On 6/22/19 12:00 PM, Denis Kirjanov wrote:
>> @@ -365,13 +367,45 @@ static void print_vfinfo(FILE *fp, struct rtattr
>> *vfinfo)
>> parse_rtattr_nested(vf, IFLA_VF_MAX, vfinfo);
>>
>> vf_mac = RTA_DATA(vf[IFLA_VF_MAC]);
>> + vf_broadcast = RTA_DATA(vf[IFLA_VF_BROADCAST]);
>> vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
>>
>> print_string(PRINT_FP, NULL, "%s ", _SL_);
>> print_int(PRINT_ANY, "vf", "vf %d ", vf_mac->vf);
>> - print_string(PRINT_ANY, "mac", "MAC %s",
>> - ll_addr_n2a((unsigned char *) &vf_mac->mac,
>> - ETH_ALEN, 0, b1, sizeof(b1)));
>> +
>> + print_string(PRINT_ANY,
>> + "link_type",
>> + " link/%s ",
>> + ll_type_n2a(ifi->ifi_type, b1, sizeof(b1)));
>> +
>> + print_color_string(PRINT_ANY,
>> + COLOR_MAC,
>> + "address",
>> + "%s",
>> + ll_addr_n2a((unsigned char *) &vf_mac->mac,
>> + ifi->ifi_type == ARPHRD_ETHER ?
>> + ETH_ALEN : INFINIBAND_ALEN,
>> + ifi->ifi_type,
>> + b1, sizeof(b1)));
>
> you still have a lot of lines that are not lined up column wise. See how
> the COLOR_MAC is offset to the right from PRINT_ANY?
>
>> +
>> + if (vf[IFLA_VF_BROADCAST]) {
>> + if (ifi->ifi_flags&IFF_POINTOPOINT) {
>> + print_string(PRINT_FP, NULL, " peer ", NULL);
>> + print_bool(PRINT_JSON,
>> + "link_pointtopoint", NULL, true);
>> + } else
>> + print_string(PRINT_FP, NULL, " brd ", NULL);
>> +
>> + print_color_string(PRINT_ANY,
>> + COLOR_MAC,
>> + "broadcast",
>> + "%s",
>> + ll_addr_n2a((unsigned char *) &vf_broadcast->broadcast,
>> + ifi->ifi_type == ARPHRD_ETHER ?
>> + ETH_ALEN : INFINIBAND_ALEN,
>> + ifi->ifi_type,
>> + b1, sizeof(b1)));
>
> And then these lines are offset to the left.
Hi David,
I've just sent a new version and I've formatted strings in the similar
way as it used through the source.
Thank you.
>
^ permalink raw reply
* Re: [PATCH bpf-next v5 2/3] bpf_xdp_redirect_map: Perform map lookup in eBPF helper
From: Toke Høiland-Jørgensen @ 2019-06-28 8:57 UTC (permalink / raw)
To: Daniel Borkmann, netdev
Cc: Jesper Dangaard Brouer, Alexei Starovoitov, David Miller,
Jonathan Lemon
In-Reply-To: <87pnmy86mm.fsf@toke.dk>
Toke Høiland-Jørgensen <toke@redhat.com> writes:
> Daniel Borkmann <daniel@iogearbox.net> writes:
>
>> On 06/28/2019 09:17 AM, Toke Høiland-Jørgensen wrote:
>>> Daniel Borkmann <daniel@iogearbox.net> writes:
>>>
>>>> On 06/23/2019 04:17 AM, Toke Høiland-Jørgensen wrote:
>>>>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>>
>>>>> The bpf_redirect_map() helper used by XDP programs doesn't return any
>>>>> indication of whether it can successfully redirect to the map index it was
>>>>> given. Instead, BPF programs have to track this themselves, leading to
>>>>> programs using duplicate maps to track which entries are populated in the
>>>>> devmap.
>>>>>
>>>>> This patch fixes this by moving the map lookup into the bpf_redirect_map()
>>>>> helper, which makes it possible to return failure to the eBPF program. The
>>>>> lower bits of the flags argument is used as the return code, which means
>>>>> that existing users who pass a '0' flag argument will get XDP_ABORTED.
>>>>>
>>>>> With this, a BPF program can check the return code from the helper call and
>>>>> react by, for instance, substituting a different redirect. This works for
>>>>> any type of map used for redirect.
>>>>>
>>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>
>>>> Overall series looks good to me. Just very small things inline here & in the
>>>> other two patches:
>>>>
>>>> [...]
>>>>> @@ -3750,9 +3742,16 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
>>>>> {
>>>>> struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>>>>>
>>>>> - if (unlikely(flags))
>>>>> + /* Lower bits of the flags are used as return code on lookup failure */
>>>>> + if (unlikely(flags > XDP_TX))
>>>>> return XDP_ABORTED;
>>>>>
>>>>> + ri->item = __xdp_map_lookup_elem(map, ifindex);
>>>>> + if (unlikely(!ri->item)) {
>>>>> + WRITE_ONCE(ri->map, NULL);
>>>>
>>>> This WRITE_ONCE() is not needed. We never set it before at this point.
>>>
>>> You mean the WRITE_ONCE() wrapper is not needed, or the set-to-NULL is
>>> not needed? The reason I added it is in case an eBPF program calls the
>>> helper twice before returning, where the first lookup succeeds but the
>>> second fails; in that case we want to clear the ->map pointer, no?
>>
>> Yeah I meant the set-to-NULL. So if first call succeeds, and the second one
>> fails, then the expected semantics wrt the first call are as if the program
>> would have called bpf_xdp_redirect() only?
>>
>> Looking at the code again, if we set ri->item to NULL, then we /must/
>> also set ri->map to NULL. I guess there are two options: i) leave as
>> is, ii) keep the __xdp_map_lookup_elem() result in a temp var, if it's
>> NULL return flags, otherwise only /then/ update ri->item, so that
>> semantics are similar to the invalid flags check earlier. I guess fine
>> either way, in case of i) there should probably be a comment since
>> it's less obvious.
>
> Yeah, I think a temp var is probably clearer, will do that :)
Actually, thinking about this some more, I think it's better to
completely clear out the state the second time around. I'll add a
comment to explain.
-Toke
^ permalink raw reply
* Re: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
From: Benjamin Poirier @ 2019-06-28 8:57 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Manish Chopra, GR-Linux-NIC-Dev, netdev@vger.kernel.org
In-Reply-To: <CA+FuTSfKw6aaXk0hA0p_AUp9Oa_D+5Bwst8HUz7mJM-wO5Obow@mail.gmail.com>
On 2019/06/26 11:42, Willem de Bruijn wrote:
> On Wed, Jun 26, 2019 at 7:37 AM Benjamin Poirier <bpoirier@suse.com> wrote:
> >
> > On 2019/06/26 09:24, Manish Chopra wrote:
> > > > -----Original Message-----
> > > > From: Benjamin Poirier <bpoirier@suse.com>
> > > > Sent: Monday, June 17, 2019 1:19 PM
> > > > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > > > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > > > Subject: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
> > > >
> > > > External Email
> > > >
> > > > ----------------------------------------------------------------------
> > > > lbq_buf_size is duplicated to every rx_ring structure whereas lbq_buf_order is
> > > > present once in the ql_adapter structure. All rings use the same buf size, keep
> > > > only one copy of it. Also factor out the calculation of lbq_buf_size instead of
> > > > having two copies.
> > > >
> > > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > > > ---
> > [...]
> > >
> > > Not sure if this change is really required, I think fields relevant to rx_ring should be present in the rx_ring structure.
> > > There are various other fields like "lbq_len" and "lbq_size" which would be same for all rx rings but still under the relevant rx_ring structure.
>
> The one argument against deduplicating might be if the original fields
> are in a hot cacheline and the new location adds a cacheline access to
> a hot path. Not sure if that is relevant here. But maybe something to
> double check.
>
Thanks for the hint. I didn't check before because my hunch was that
this driver is not near that level of optimization but I checked now and
got the following results.
The other side of the link is doing pktgen of 257B frames (smallest to
exercise the lbq path), over 10 runs of 100s, number of frames received
by the stack before/after applying this patch:
before: 1487000±2000pps
after: 1538000±2000pps
(3.4% improvement)
Looking with perf stat, boiling down the numbers to be per packet, most
interesting differences are:
cycles -4.32%
instructions -0.97%
L1-dcache-loads +6.30%
L1-dcache-load-misses -1.26%
LLC-loads +2.93%
LLC-load-misses -1.16%
So, fewer cycles/packet, fewer cache misses. Note however that
L1-dcache-loads had high variability between runs (~12%).
Looking with pahole, struct rx_ring is reduced by 8 bytes but its a 784
bytes structure full of holes to begin with.
Numbers are from an old Xeon E3-1275
Full output follows:
root@dtest:~# cat /tmp/mini.sh
#!/bin/bash
da=$(ethtool -S ql1 | awk '/rx_nic_fifo_drop/ {print $2}')
ra=$(ip -s link show dev ql1 | awk 'NR == 4 {print $2}')
sleep 100
db=$(ethtool -S ql1 | awk '/rx_nic_fifo_drop/ {print $2}')
rb=$(ip -s link show dev ql1 | awk 'NR == 4 {print $2}')
echo "rx $((rb - ra))"
echo "dropped $((db - da))"
root@dtest:~# perf stat -a -r10 -d /tmp/mini.sh
rx 148763519
dropped 296128797
rx 148784865
dropped 296107878
rx 148675349
dropped 296217234
rx 148634271
dropped 296260358
rx 148813581
dropped 296089785
rx 148755713
dropped 296136071
rx 148434116
dropped 296459743
rx 148390638
dropped 296501405
rx 148500812
dropped 296391286
rx 148973912
dropped 295920014
Performance counter stats for 'system wide' (10 runs):
800,245.24 msec cpu-clock # 8.000 CPUs utilized ( +- 0.00% )
11,559 context-switches # 14.445 M/sec ( +- 0.13% )
36 cpu-migrations # 0.044 M/sec ( +- 7.11% )
62,100 page-faults # 77.601 M/sec ( +- 0.51% )
362,221,639,977 cycles # 452638.486 GHz ( +- 0.77% ) (49.92%)
515,318,132,327 instructions # 1.42 insn per cycle ( +- 0.64% ) (62.42%)
91,744,969,260 branches # 114646115.533 M/sec ( +- 0.04% ) (62.50%)
549,681,601 branch-misses # 0.60% of all branches ( +- 0.25% ) (62.50%)
115,207,692,963 L1-dcache-loads # 143965544.751 M/sec ( +- 12.09% ) (40.59%)
2,629,809,584 L1-dcache-load-misses # 2.28% of all L1-dcache hits ( +- 3.24% ) (29.90%)
837,806,984 LLC-loads # 1046938.235 M/sec ( +- 0.73% ) (25.31%)
569,343,079 LLC-load-misses # 67.96% of all LL-cache hits ( +- 0.07% ) (37.50%)
100.03177 +- 0.00104 seconds time elapsed ( +- 0.00% )
#
# changed the driver now to include this patch
#
root@dtest:~# perf stat -a -r10 -d /tmp/mini.sh
rx 154094001
dropped 290799944
rx 153798301
dropped 291094799
rx 153942166
dropped 290950792
rx 154150506
dropped 290743194
rx 154179791
dropped 290712400
rx 153907213
dropped 290985227
rx 153813515
dropped 291078475
rx 153603554
dropped 291289250
rx 153521414
dropped 291372145
rx 153592955
dropped 291299734
Performance counter stats for 'system wide' (10 runs):
800,234.85 msec cpu-clock # 8.000 CPUs utilized ( +- 0.00% )
12,061 context-switches # 15.072 M/sec ( +- 0.61% )
32 cpu-migrations # 0.040 M/sec ( +- 8.40% )
62,210 page-faults # 77.740 M/sec ( +- 0.60% )
358,454,546,598 cycles # 447936.882 GHz ( +- 1.34% ) (49.84%)
527,804,259,267 instructions # 1.47 insn per cycle ( +- 1.11% ) (62.35%)
94,904,030,904 branches # 118595275.389 M/sec ( +- 0.05% ) (62.50%)
568,214,180 branch-misses # 0.60% of all branches ( +- 0.12% ) (62.50%)
126,667,377,528 L1-dcache-loads # 158287823.791 M/sec ( +- 12.48% ) (41.21%)
2,685,865,791 L1-dcache-load-misses # 2.12% of all L1-dcache hits ( +- 4.01% ) (29.42%)
891,939,954 LLC-loads # 1114598.225 M/sec ( +- 0.73% ) (25.16%)
582,037,612 LLC-load-misses # 65.26% of all LL-cache hits ( +- 0.09% ) (37.50%)
100.0307117 +- 0.0000810 seconds time elapsed ( +- 0.00% )
^ permalink raw reply
* Re: [PATCH net-next] net: ipvlan: forward ingress packet to slave's l2 in l3s mode
From: Zhiyuan Hou @ 2019-06-28 8:59 UTC (permalink / raw)
To: Paolo Abeni, davem, idosch, daniel, petrm, jiri, tglx, linmiaohe
Cc: zhabin, caspar, netdev, linux-kernel, wei.yang1
In-Reply-To: <24fab1f43190f4994e47da4c2fa3fd622cd4e8ca.camel@redhat.com>
在 2019/6/26 下午4:16, Paolo Abeni 写道:
> Hi,
>
> On Tue, 2019-06-25 at 14:42 +0800, Zhiyuan Hou wrote:
>> In ipvlan l3s mode, ingress packet is switched to slave interface and
>> delivers to l4 stack. This may cause two problems:
>>
>> 1. When slave is in an ns different from master, the behavior of stack
>> in slave ns may cause confusion for users. For example, iptables, tc,
>> and other l2/l3 functions are not available for ingress packet.
>>
>> 2. l3s mode is not used for tap device, and cannot support ipvtap. But
>> in VM or container based VM cases, tap device is a very common device.
>>
>> In l3s mode's input nf_hook, this patch calles the skb_forward_dev() to
>> forward ingress packet to slave and uses nf_conntrack_confirm() to make
>> conntrack work with new mode.
>>
>> Signed-off-by: Zha Bin <zhabin@linux.alibaba.com>
>> Signed-off-by: Zhiyuan Hou <zhiyuan2048@linux.alibaba.com>
>> ---
>> drivers/net/ipvlan/ipvlan.h | 9 ++++++++-
>> drivers/net/ipvlan/ipvlan_l3s.c | 16 ++++++++++++++--
>> 2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ipvlan/ipvlan.h b/drivers/net/ipvlan/ipvlan.h
>> index 3837c897832e..48c814e24c3f 100644
>> --- a/drivers/net/ipvlan/ipvlan.h
>> +++ b/drivers/net/ipvlan/ipvlan.h
>> @@ -172,6 +172,14 @@ void ipvlan_link_delete(struct net_device *dev, struct list_head *head);
>> void ipvlan_link_setup(struct net_device *dev);
>> int ipvlan_link_register(struct rtnl_link_ops *ops);
>> #ifdef CONFIG_IPVLAN_L3S
>> +
>> +#include <net/netfilter/nf_conntrack_core.h>
>> +
>> +static inline int ipvlan_confirm_conntrack(struct sk_buff *skb)
>> +{
>> + return nf_conntrack_confirm(skb);
>> +}
>> +
>> int ipvlan_l3s_register(struct ipvl_port *port);
>> void ipvlan_l3s_unregister(struct ipvl_port *port);
>> void ipvlan_migrate_l3s_hook(struct net *oldnet, struct net *newnet);
>> @@ -206,5 +214,4 @@ static inline bool netif_is_ipvlan_port(const struct net_device *dev)
>> {
>> return rcu_access_pointer(dev->rx_handler) == ipvlan_handle_frame;
>> }
>> -
>> #endif /* __IPVLAN_H */
>> diff --git a/drivers/net/ipvlan/ipvlan_l3s.c b/drivers/net/ipvlan/ipvlan_l3s.c
>> index 943d26cbf39f..ed210002f593 100644
>> --- a/drivers/net/ipvlan/ipvlan_l3s.c
>> +++ b/drivers/net/ipvlan/ipvlan_l3s.c
>> @@ -95,14 +95,26 @@ static unsigned int ipvlan_nf_input(void *priv, struct sk_buff *skb,
>> {
>> struct ipvl_addr *addr;
>> unsigned int len;
>> + int ret = NF_ACCEPT;
>> + bool success;
>>
>> addr = ipvlan_skb_to_addr(skb, skb->dev);
>> if (!addr)
>> goto out;
>>
>> - skb->dev = addr->master->dev;
>> len = skb->len + ETH_HLEN;
>> - ipvlan_count_rx(addr->master, len, true, false);
>> +
>> + ret = ipvlan_confirm_conntrack(skb);
>> + if (ret != NF_ACCEPT) {
>> + ipvlan_count_rx(addr->master, len, false, false);
>> + goto out;
>> + }
>> +
>> + skb_push_rcsum(skb, ETH_HLEN);
>> + success = dev_forward_skb(addr->master->dev, skb) == NET_RX_SUCCESS;
> This looks weird to me: if I read the code correctly, the skb will
> traverse twice NF_INET_LOCAL_IN, once due to the l3s hooking and
> another one due to dev_forward_skb().
>
> Also, tc ingreess, etc will run after the first traversing of
> NF_INET_LOCAL_IN.
Yes, but the skb's device has been modified from the master to slave.
In most use cases of
ipvlan, the master device and slave device are in different namespace
(ns), so the second
triggered LOCAL_IN is completely isolated from the first triggered
LOCAL_IN.
When the master device and slave device are in the same ns, the behavior
of this patch is
similar to that of L2 over L3 tunnel (forwarding from L3 to L2 device).
Since the device has been modified, the second triggered tc-ingress is
thus different.
>
> All in all I think that if full l2 processing is required, a different
> mode or a different virtual device should be used.
We can implement it in a new mode, but such a way is similar to the
current ipvlan l3s mode.
Also, ipvlan l3s mode has two problems described in patch's commit log.
I think that a more
appropriate solution is to modify ipvlan l3s.
> Cheers,
>
> Paolo
^ permalink raw reply
* Re: [PATCH v2 bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf
From: Lorenz Bauer @ 2019-06-28 9:01 UTC (permalink / raw)
To: Song Liu
Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team,
Jann Horn, gregkh
In-Reply-To: <20190627201923.2589391-2-songliubraving@fb.com>
On Thu, 27 Jun 2019 at 21:19, Song Liu <songliubraving@fb.com> wrote:
>
> This patch introduce unprivileged BPF access. The access control is
> achieved via device /dev/bpf. Users with write access to /dev/bpf are able
> to call sys_bpf().
>
> Two ioctl command are added to /dev/bpf:
>
> The two commands enable/disable permission to call sys_bpf() for current
> task. This permission is noted by bpf_permitted in task_struct. This
> permission is inherited during clone(CLONE_THREAD).
If I understand it correctly, a process would have to open /dev/bpf before
spawning other threads for this to work?
That still wouldn't work for Go I'm afraid. The runtime creates and destroys
threads on an ad-hoc basis, and there is no way to "initialize" in the
first thread.
With the API as is, any Go wrapper wishing to use this would have to do the
following _for every BPF syscall_:
1. Use runtime.LockOSThread() to prevent the scheduler from moving the
goroutine.
2. Open /dev/bpf to set the bit in current_task
3. Execute the syscall
4. Call runtime.UnlockOSThread()
Note that calling into C code via CGo doesn't change this. Is it not possible to
set the bit on all processes in the current thread group?
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH v2 bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf
From: Lorenz Bauer @ 2019-06-28 9:05 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Song Liu, Networking, bpf, Alexei Starovoitov, Daniel Borkmann,
Kernel Team, Jann Horn, gregkh, linux-abi, kees
In-Reply-To: <21894f45-70d8-dfca-8c02-044f776c5e05@kernel.org>
On Fri, 28 Jun 2019 at 00:40, Andy Lutomirski <luto@kernel.org> wrote:
>
> I have a bigger issue with this patch, though: it's a really awkward way
> to pretend to have capabilities. For bpf, it seems like you could make
> this be a *real* capability without too much pain since there's only one
> syscall there. Just find a way to pass an fd to /dev/bpf into the
> syscall. If this means you need a new bpf_with_cap() syscall that takes
> an extra argument, so be it. The old bpf() syscall can just translate
> to bpf_with_cap(..., -1).
I agree, this seems nicer from my POV, since it evades the issues with
the Go runtime I pointed out in the other message.
It also seems like this wouldn't have to create API churn in libbpf? We can
"feature detect" the presence of the new syscall and use that instead. If
you want you can even keep the semantics of having a "global" credential.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
^ permalink raw reply
* [PATCH bpf-next v6 2/5] devmap/cpumap: Use flush list instead of bitmap
From: Toke Høiland-Jørgensen @ 2019-06-28 9:12 UTC (permalink / raw)
To: netdev
Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
David Miller, Jonathan Lemon
In-Reply-To: <156171315462.9468.3367572649463706996.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
The socket map uses a linked list instead of a bitmap to keep track of
which entries to flush. Do the same for devmap and cpumap, as this means we
don't have to care about the map index when enqueueing things into the
map (and so we can cache the map lookup).
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
---
kernel/bpf/cpumap.c | 105 +++++++++++++++++++++++---------------------------
kernel/bpf/devmap.c | 107 ++++++++++++++++++++++-----------------------------
net/core/filter.c | 2 -
3 files changed, 95 insertions(+), 119 deletions(-)
diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index 8dff08768087..ef49e17ae47c 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -32,14 +32,19 @@
/* General idea: XDP packets getting XDP redirected to another CPU,
* will maximum be stored/queued for one driver ->poll() call. It is
- * guaranteed that setting flush bit and flush operation happen on
+ * guaranteed that queueing the frame and the flush operation happen on
* same CPU. Thus, cpu_map_flush operation can deduct via this_cpu_ptr()
* which queue in bpf_cpu_map_entry contains packets.
*/
#define CPU_MAP_BULK_SIZE 8 /* 8 == one cacheline on 64-bit archs */
+struct bpf_cpu_map_entry;
+struct bpf_cpu_map;
+
struct xdp_bulk_queue {
void *q[CPU_MAP_BULK_SIZE];
+ struct list_head flush_node;
+ struct bpf_cpu_map_entry *obj;
unsigned int count;
};
@@ -52,6 +57,8 @@ struct bpf_cpu_map_entry {
/* XDP can run multiple RX-ring queues, need __percpu enqueue store */
struct xdp_bulk_queue __percpu *bulkq;
+ struct bpf_cpu_map *cmap;
+
/* Queue with potential multi-producers, and single-consumer kthread */
struct ptr_ring *queue;
struct task_struct *kthread;
@@ -65,23 +72,17 @@ struct bpf_cpu_map {
struct bpf_map map;
/* Below members specific for map type */
struct bpf_cpu_map_entry **cpu_map;
- unsigned long __percpu *flush_needed;
+ struct list_head __percpu *flush_list;
};
-static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
- struct xdp_bulk_queue *bq, bool in_napi_ctx);
-
-static u64 cpu_map_bitmap_size(const union bpf_attr *attr)
-{
- return BITS_TO_LONGS(attr->max_entries) * sizeof(unsigned long);
-}
+static int bq_flush_to_queue(struct xdp_bulk_queue *bq, bool in_napi_ctx);
static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
{
struct bpf_cpu_map *cmap;
int err = -ENOMEM;
+ int ret, cpu;
u64 cost;
- int ret;
if (!capable(CAP_SYS_ADMIN))
return ERR_PTR(-EPERM);
@@ -105,7 +106,7 @@ static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
/* make sure page count doesn't overflow */
cost = (u64) cmap->map.max_entries * sizeof(struct bpf_cpu_map_entry *);
- cost += cpu_map_bitmap_size(attr) * num_possible_cpus();
+ cost += sizeof(struct list_head) * num_possible_cpus();
/* Notice returns -EPERM on if map size is larger than memlock limit */
ret = bpf_map_charge_init(&cmap->map.memory, cost);
@@ -114,12 +115,13 @@ static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
goto free_cmap;
}
- /* A per cpu bitfield with a bit per possible CPU in map */
- cmap->flush_needed = __alloc_percpu(cpu_map_bitmap_size(attr),
- __alignof__(unsigned long));
- if (!cmap->flush_needed)
+ cmap->flush_list = alloc_percpu(struct list_head);
+ if (!cmap->flush_list)
goto free_charge;
+ for_each_possible_cpu(cpu)
+ INIT_LIST_HEAD(per_cpu_ptr(cmap->flush_list, cpu));
+
/* Alloc array for possible remote "destination" CPUs */
cmap->cpu_map = bpf_map_area_alloc(cmap->map.max_entries *
sizeof(struct bpf_cpu_map_entry *),
@@ -129,7 +131,7 @@ static struct bpf_map *cpu_map_alloc(union bpf_attr *attr)
return &cmap->map;
free_percpu:
- free_percpu(cmap->flush_needed);
+ free_percpu(cmap->flush_list);
free_charge:
bpf_map_charge_finish(&cmap->map.memory);
free_cmap:
@@ -334,7 +336,8 @@ static struct bpf_cpu_map_entry *__cpu_map_entry_alloc(u32 qsize, u32 cpu,
{
gfp_t gfp = GFP_KERNEL | __GFP_NOWARN;
struct bpf_cpu_map_entry *rcpu;
- int numa, err;
+ struct xdp_bulk_queue *bq;
+ int numa, err, i;
/* Have map->numa_node, but choose node of redirect target CPU */
numa = cpu_to_node(cpu);
@@ -349,6 +352,11 @@ static struct bpf_cpu_map_entry *__cpu_map_entry_alloc(u32 qsize, u32 cpu,
if (!rcpu->bulkq)
goto free_rcu;
+ for_each_possible_cpu(i) {
+ bq = per_cpu_ptr(rcpu->bulkq, i);
+ bq->obj = rcpu;
+ }
+
/* Alloc queue */
rcpu->queue = kzalloc_node(sizeof(*rcpu->queue), gfp, numa);
if (!rcpu->queue)
@@ -405,7 +413,7 @@ static void __cpu_map_entry_free(struct rcu_head *rcu)
struct xdp_bulk_queue *bq = per_cpu_ptr(rcpu->bulkq, cpu);
/* No concurrent bq_enqueue can run at this point */
- bq_flush_to_queue(rcpu, bq, false);
+ bq_flush_to_queue(bq, false);
}
free_percpu(rcpu->bulkq);
/* Cannot kthread_stop() here, last put free rcpu resources */
@@ -488,6 +496,7 @@ static int cpu_map_update_elem(struct bpf_map *map, void *key, void *value,
rcpu = __cpu_map_entry_alloc(qsize, key_cpu, map->id);
if (!rcpu)
return -ENOMEM;
+ rcpu->cmap = cmap;
}
rcu_read_lock();
__cpu_map_entry_replace(cmap, key_cpu, rcpu);
@@ -514,14 +523,14 @@ static void cpu_map_free(struct bpf_map *map)
synchronize_rcu();
/* To ensure all pending flush operations have completed wait for flush
- * bitmap to indicate all flush_needed bits to be zero on _all_ cpus.
- * Because the above synchronize_rcu() ensures the map is disconnected
- * from the program we can assume no new bits will be set.
+ * list be empty on _all_ cpus. Because the above synchronize_rcu()
+ * ensures the map is disconnected from the program we can assume no new
+ * items will be added to the list.
*/
for_each_online_cpu(cpu) {
- unsigned long *bitmap = per_cpu_ptr(cmap->flush_needed, cpu);
+ struct list_head *flush_list = per_cpu_ptr(cmap->flush_list, cpu);
- while (!bitmap_empty(bitmap, cmap->map.max_entries))
+ while (!list_empty(flush_list))
cond_resched();
}
@@ -538,7 +547,7 @@ static void cpu_map_free(struct bpf_map *map)
/* bq flush and cleanup happens after RCU graze-period */
__cpu_map_entry_replace(cmap, i, NULL); /* call_rcu */
}
- free_percpu(cmap->flush_needed);
+ free_percpu(cmap->flush_list);
bpf_map_area_free(cmap->cpu_map);
kfree(cmap);
}
@@ -590,9 +599,9 @@ const struct bpf_map_ops cpu_map_ops = {
.map_check_btf = map_check_no_btf,
};
-static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
- struct xdp_bulk_queue *bq, bool in_napi_ctx)
+static int bq_flush_to_queue(struct xdp_bulk_queue *bq, bool in_napi_ctx)
{
+ struct bpf_cpu_map_entry *rcpu = bq->obj;
unsigned int processed = 0, drops = 0;
const int to_cpu = rcpu->cpu;
struct ptr_ring *q;
@@ -621,6 +630,8 @@ static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
bq->count = 0;
spin_unlock(&q->producer_lock);
+ __list_del_clearprev(&bq->flush_node);
+
/* Feedback loop via tracepoints */
trace_xdp_cpumap_enqueue(rcpu->map_id, processed, drops, to_cpu);
return 0;
@@ -631,10 +642,11 @@ static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
*/
static int bq_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_frame *xdpf)
{
+ struct list_head *flush_list = this_cpu_ptr(rcpu->cmap->flush_list);
struct xdp_bulk_queue *bq = this_cpu_ptr(rcpu->bulkq);
if (unlikely(bq->count == CPU_MAP_BULK_SIZE))
- bq_flush_to_queue(rcpu, bq, true);
+ bq_flush_to_queue(bq, true);
/* Notice, xdp_buff/page MUST be queued here, long enough for
* driver to code invoking us to finished, due to driver
@@ -646,6 +658,10 @@ static int bq_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_frame *xdpf)
* operation, when completing napi->poll call.
*/
bq->q[bq->count++] = xdpf;
+
+ if (!bq->flush_node.prev)
+ list_add(&bq->flush_node, flush_list);
+
return 0;
}
@@ -665,41 +681,16 @@ int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp,
return 0;
}
-void __cpu_map_insert_ctx(struct bpf_map *map, u32 bit)
-{
- struct bpf_cpu_map *cmap = container_of(map, struct bpf_cpu_map, map);
- unsigned long *bitmap = this_cpu_ptr(cmap->flush_needed);
-
- __set_bit(bit, bitmap);
-}
-
void __cpu_map_flush(struct bpf_map *map)
{
struct bpf_cpu_map *cmap = container_of(map, struct bpf_cpu_map, map);
- unsigned long *bitmap = this_cpu_ptr(cmap->flush_needed);
- u32 bit;
-
- /* The napi->poll softirq makes sure __cpu_map_insert_ctx()
- * and __cpu_map_flush() happen on same CPU. Thus, the percpu
- * bitmap indicate which percpu bulkq have packets.
- */
- for_each_set_bit(bit, bitmap, map->max_entries) {
- struct bpf_cpu_map_entry *rcpu = READ_ONCE(cmap->cpu_map[bit]);
- struct xdp_bulk_queue *bq;
-
- /* This is possible if entry is removed by user space
- * between xdp redirect and flush op.
- */
- if (unlikely(!rcpu))
- continue;
-
- __clear_bit(bit, bitmap);
+ struct list_head *flush_list = this_cpu_ptr(cmap->flush_list);
+ struct xdp_bulk_queue *bq, *tmp;
- /* Flush all frames in bulkq to real queue */
- bq = this_cpu_ptr(rcpu->bulkq);
- bq_flush_to_queue(rcpu, bq, true);
+ list_for_each_entry_safe(bq, tmp, flush_list, flush_node) {
+ bq_flush_to_queue(bq, true);
/* If already running, costs spin_lock_irqsave + smb_mb */
- wake_up_process(rcpu->kthread);
+ wake_up_process(bq->obj->kthread);
}
}
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 40e86a7e0ef0..a4dddc867cbf 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -17,9 +17,8 @@
* datapath always has a valid copy. However, the datapath does a "flush"
* operation that pushes any pending packets in the driver outside the RCU
* critical section. Each bpf_dtab_netdev tracks these pending operations using
- * an atomic per-cpu bitmap. The bpf_dtab_netdev object will not be destroyed
- * until all bits are cleared indicating outstanding flush operations have
- * completed.
+ * a per-cpu flush list. The bpf_dtab_netdev object will not be destroyed until
+ * this list is empty, indicating outstanding flush operations have completed.
*
* BPF syscalls may race with BPF program calls on any of the update, delete
* or lookup operations. As noted above the xchg() operation also keep the
@@ -48,9 +47,13 @@
(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
#define DEV_MAP_BULK_SIZE 16
+struct bpf_dtab_netdev;
+
struct xdp_bulk_queue {
struct xdp_frame *q[DEV_MAP_BULK_SIZE];
+ struct list_head flush_node;
struct net_device *dev_rx;
+ struct bpf_dtab_netdev *obj;
unsigned int count;
};
@@ -65,23 +68,18 @@ struct bpf_dtab_netdev {
struct bpf_dtab {
struct bpf_map map;
struct bpf_dtab_netdev **netdev_map;
- unsigned long __percpu *flush_needed;
+ struct list_head __percpu *flush_list;
struct list_head list;
};
static DEFINE_SPINLOCK(dev_map_lock);
static LIST_HEAD(dev_map_list);
-static u64 dev_map_bitmap_size(const union bpf_attr *attr)
-{
- return BITS_TO_LONGS((u64) attr->max_entries) * sizeof(unsigned long);
-}
-
static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
{
struct bpf_dtab *dtab;
+ int err, cpu;
u64 cost;
- int err;
if (!capable(CAP_NET_ADMIN))
return ERR_PTR(-EPERM);
@@ -99,7 +97,7 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
/* make sure page count doesn't overflow */
cost = (u64) dtab->map.max_entries * sizeof(struct bpf_dtab_netdev *);
- cost += dev_map_bitmap_size(attr) * num_possible_cpus();
+ cost += sizeof(struct list_head) * num_possible_cpus();
/* if map size is larger than memlock limit, reject it */
err = bpf_map_charge_init(&dtab->map.memory, cost);
@@ -108,28 +106,30 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
err = -ENOMEM;
- /* A per cpu bitfield with a bit per possible net device */
- dtab->flush_needed = __alloc_percpu_gfp(dev_map_bitmap_size(attr),
- __alignof__(unsigned long),
- GFP_KERNEL | __GFP_NOWARN);
- if (!dtab->flush_needed)
+ dtab->flush_list = alloc_percpu(struct list_head);
+ if (!dtab->flush_list)
goto free_charge;
+ for_each_possible_cpu(cpu)
+ INIT_LIST_HEAD(per_cpu_ptr(dtab->flush_list, cpu));
+
dtab->netdev_map = bpf_map_area_alloc(dtab->map.max_entries *
sizeof(struct bpf_dtab_netdev *),
dtab->map.numa_node);
if (!dtab->netdev_map)
- goto free_charge;
+ goto free_percpu;
spin_lock(&dev_map_lock);
list_add_tail_rcu(&dtab->list, &dev_map_list);
spin_unlock(&dev_map_lock);
return &dtab->map;
+
+free_percpu:
+ free_percpu(dtab->flush_list);
free_charge:
bpf_map_charge_finish(&dtab->map.memory);
free_dtab:
- free_percpu(dtab->flush_needed);
kfree(dtab);
return ERR_PTR(err);
}
@@ -158,14 +158,14 @@ static void dev_map_free(struct bpf_map *map)
rcu_barrier();
/* To ensure all pending flush operations have completed wait for flush
- * bitmap to indicate all flush_needed bits to be zero on _all_ cpus.
+ * list to empty on _all_ cpus.
* Because the above synchronize_rcu() ensures the map is disconnected
- * from the program we can assume no new bits will be set.
+ * from the program we can assume no new items will be added.
*/
for_each_online_cpu(cpu) {
- unsigned long *bitmap = per_cpu_ptr(dtab->flush_needed, cpu);
+ struct list_head *flush_list = per_cpu_ptr(dtab->flush_list, cpu);
- while (!bitmap_empty(bitmap, dtab->map.max_entries))
+ while (!list_empty(flush_list))
cond_resched();
}
@@ -181,7 +181,7 @@ static void dev_map_free(struct bpf_map *map)
kfree(dev);
}
- free_percpu(dtab->flush_needed);
+ free_percpu(dtab->flush_list);
bpf_map_area_free(dtab->netdev_map);
kfree(dtab);
}
@@ -203,18 +203,10 @@ static int dev_map_get_next_key(struct bpf_map *map, void *key, void *next_key)
return 0;
}
-void __dev_map_insert_ctx(struct bpf_map *map, u32 bit)
-{
- struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
- unsigned long *bitmap = this_cpu_ptr(dtab->flush_needed);
-
- __set_bit(bit, bitmap);
-}
-
-static int bq_xmit_all(struct bpf_dtab_netdev *obj,
- struct xdp_bulk_queue *bq, u32 flags,
+static int bq_xmit_all(struct xdp_bulk_queue *bq, u32 flags,
bool in_napi_ctx)
{
+ struct bpf_dtab_netdev *obj = bq->obj;
struct net_device *dev = obj->dev;
int sent = 0, drops = 0, err = 0;
int i;
@@ -241,6 +233,7 @@ static int bq_xmit_all(struct bpf_dtab_netdev *obj,
trace_xdp_devmap_xmit(&obj->dtab->map, obj->bit,
sent, drops, bq->dev_rx, dev, err);
bq->dev_rx = NULL;
+ __list_del_clearprev(&bq->flush_node);
return 0;
error:
/* If ndo_xdp_xmit fails with an errno, no frames have been
@@ -263,31 +256,18 @@ static int bq_xmit_all(struct bpf_dtab_netdev *obj,
* from the driver before returning from its napi->poll() routine. The poll()
* routine is called either from busy_poll context or net_rx_action signaled
* from NET_RX_SOFTIRQ. Either way the poll routine must complete before the
- * net device can be torn down. On devmap tear down we ensure the ctx bitmap
- * is zeroed before completing to ensure all flush operations have completed.
+ * net device can be torn down. On devmap tear down we ensure the flush list
+ * is empty before completing to ensure all flush operations have completed.
*/
void __dev_map_flush(struct bpf_map *map)
{
struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
- unsigned long *bitmap = this_cpu_ptr(dtab->flush_needed);
- u32 bit;
+ struct list_head *flush_list = this_cpu_ptr(dtab->flush_list);
+ struct xdp_bulk_queue *bq, *tmp;
rcu_read_lock();
- for_each_set_bit(bit, bitmap, map->max_entries) {
- struct bpf_dtab_netdev *dev = READ_ONCE(dtab->netdev_map[bit]);
- struct xdp_bulk_queue *bq;
-
- /* This is possible if the dev entry is removed by user space
- * between xdp redirect and flush op.
- */
- if (unlikely(!dev))
- continue;
-
- bq = this_cpu_ptr(dev->bulkq);
- bq_xmit_all(dev, bq, XDP_XMIT_FLUSH, true);
-
- __clear_bit(bit, bitmap);
- }
+ list_for_each_entry_safe(bq, tmp, flush_list, flush_node)
+ bq_xmit_all(bq, XDP_XMIT_FLUSH, true);
rcu_read_unlock();
}
@@ -314,10 +294,11 @@ static int bq_enqueue(struct bpf_dtab_netdev *obj, struct xdp_frame *xdpf,
struct net_device *dev_rx)
{
+ struct list_head *flush_list = this_cpu_ptr(obj->dtab->flush_list);
struct xdp_bulk_queue *bq = this_cpu_ptr(obj->bulkq);
if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
- bq_xmit_all(obj, bq, 0, true);
+ bq_xmit_all(bq, 0, true);
/* Ingress dev_rx will be the same for all xdp_frame's in
* bulk_queue, because bq stored per-CPU and must be flushed
@@ -327,6 +308,10 @@ static int bq_enqueue(struct bpf_dtab_netdev *obj, struct xdp_frame *xdpf,
bq->dev_rx = dev_rx;
bq->q[bq->count++] = xdpf;
+
+ if (!bq->flush_node.prev)
+ list_add(&bq->flush_node, flush_list);
+
return 0;
}
@@ -377,17 +362,12 @@ static void dev_map_flush_old(struct bpf_dtab_netdev *dev)
{
if (dev->dev->netdev_ops->ndo_xdp_xmit) {
struct xdp_bulk_queue *bq;
- unsigned long *bitmap;
-
int cpu;
rcu_read_lock();
for_each_online_cpu(cpu) {
- bitmap = per_cpu_ptr(dev->dtab->flush_needed, cpu);
- __clear_bit(dev->bit, bitmap);
-
bq = per_cpu_ptr(dev->bulkq, cpu);
- bq_xmit_all(dev, bq, XDP_XMIT_FLUSH, false);
+ bq_xmit_all(bq, XDP_XMIT_FLUSH, false);
}
rcu_read_unlock();
}
@@ -434,8 +414,10 @@ static int dev_map_update_elem(struct bpf_map *map, void *key, void *value,
struct net *net = current->nsproxy->net_ns;
gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN;
struct bpf_dtab_netdev *dev, *old_dev;
- u32 i = *(u32 *)key;
u32 ifindex = *(u32 *)value;
+ struct xdp_bulk_queue *bq;
+ u32 i = *(u32 *)key;
+ int cpu;
if (unlikely(map_flags > BPF_EXIST))
return -EINVAL;
@@ -458,6 +440,11 @@ static int dev_map_update_elem(struct bpf_map *map, void *key, void *value,
return -ENOMEM;
}
+ for_each_possible_cpu(cpu) {
+ bq = per_cpu_ptr(dev->bulkq, cpu);
+ bq->obj = dev;
+ }
+
dev->dev = dev_get_by_index(net, ifindex);
if (!dev->dev) {
free_percpu(dev->bulkq);
diff --git a/net/core/filter.c b/net/core/filter.c
index 2014d76e0d2a..183bf4d8e301 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3523,7 +3523,6 @@ static int __bpf_tx_xdp_map(struct net_device *dev_rx, void *fwd,
err = dev_map_enqueue(dst, xdp, dev_rx);
if (unlikely(err))
return err;
- __dev_map_insert_ctx(map, index);
break;
}
case BPF_MAP_TYPE_CPUMAP: {
@@ -3532,7 +3531,6 @@ static int __bpf_tx_xdp_map(struct net_device *dev_rx, void *fwd,
err = cpu_map_enqueue(rcpu, xdp, dev_rx);
if (unlikely(err))
return err;
- __cpu_map_insert_ctx(map, index);
break;
}
case BPF_MAP_TYPE_XSKMAP: {
^ permalink raw reply related
* [PATCH bpf-next v6 0/5] xdp: Allow lookup into devmaps before redirect
From: Toke Høiland-Jørgensen @ 2019-06-28 9:12 UTC (permalink / raw)
To: netdev
Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
David Miller, Jonathan Lemon
When using the bpf_redirect_map() helper to redirect packets from XDP, the eBPF
program cannot currently know whether the redirect will succeed, which makes it
impossible to gracefully handle errors. To properly fix this will probably
require deeper changes to the way TX resources are allocated, but one thing that
is fairly straight forward to fix is to allow lookups into devmaps, so programs
can at least know when a redirect is *guaranteed* to fail because there is no
entry in the map. Currently, programs work around this by keeping a shadow map
of another type which indicates whether a map index is valid.
This series contains two changes that are complementary ways to fix this issue:
- Moving the map lookup into the bpf_redirect_map() helper (and caching the
result), so the helper can return an error if no value is found in the map.
This includes a refactoring of the devmap and cpumap code to not care about
the index on enqueue.
- Allowing regular lookups into devmaps from eBPF programs, using the read-only
flag to make sure they don't change the values.
The performance impact of the series is negligible, in the sense that I cannot
measure it because the variance between test runs is higher than the difference
pre/post series.
Changelog:
v6:
- Factor out list handling in maps to a helper in list.h (new patch 1)
- Rename variables in struct bpf_redirect_info (new patch 3 + patch 4)
- Explain why we are clearing out the map in the info struct on lookup failure
- Remove unneeded check for forwarding target in tracepoint macro
v5:
- Rebase on latest bpf-next.
- Update documentation for bpf_redirect_map() with the new meaning of flags.
v4:
- Fix a few nits from Andrii
- Lose the #defines in bpf.h and just compare the flags argument directly to
XDP_TX in bpf_xdp_redirect_map().
v3:
- Adopt Jonathan's idea of using the lower two bits of the flag value as the
return code.
- Always do the lookup, and cache the result for use in xdp_do_redirect(); to
achieve this, refactor the devmap and cpumap code to get rid the bitmap for
selecting which devices to flush.
v2:
- For patch 1, make it clear that the change works for any map type.
- For patch 2, just use the new BPF_F_RDONLY_PROG flag to make the return
value read-only.
---
Toke Høiland-Jørgensen (5):
xskmap: Move non-standard list manipulation to helper
devmap/cpumap: Use flush list instead of bitmap
devmap: Rename ifindex member in bpf_redirect_info
bpf_xdp_redirect_map: Perform map lookup in eBPF helper
devmap: Allow map lookups from eBPF
include/linux/filter.h | 3 +
include/linux/list.h | 14 ++++++
include/trace/events/xdp.h | 5 +-
include/uapi/linux/bpf.h | 7 ++-
kernel/bpf/cpumap.c | 105 +++++++++++++++++++----------------------
kernel/bpf/devmap.c | 112 ++++++++++++++++++++------------------------
kernel/bpf/verifier.c | 7 +--
kernel/bpf/xskmap.c | 3 -
net/core/filter.c | 60 ++++++++++++------------
9 files changed, 157 insertions(+), 159 deletions(-)
^ permalink raw reply
* [PATCH bpf-next v6 4/5] bpf_xdp_redirect_map: Perform map lookup in eBPF helper
From: Toke Høiland-Jørgensen @ 2019-06-28 9:12 UTC (permalink / raw)
To: netdev
Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
David Miller, Jonathan Lemon
In-Reply-To: <156171315462.9468.3367572649463706996.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
The bpf_redirect_map() helper used by XDP programs doesn't return any
indication of whether it can successfully redirect to the map index it was
given. Instead, BPF programs have to track this themselves, leading to
programs using duplicate maps to track which entries are populated in the
devmap.
This patch fixes this by moving the map lookup into the bpf_redirect_map()
helper, which makes it possible to return failure to the eBPF program. The
lower bits of the flags argument is used as the return code, which means
that existing users who pass a '0' flag argument will get XDP_ABORTED.
With this, a BPF program can check the return code from the helper call and
react by, for instance, substituting a different redirect. This works for
any type of map used for redirect.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
---
include/linux/filter.h | 1 +
include/trace/events/xdp.h | 5 ++---
include/uapi/linux/bpf.h | 7 +++++--
net/core/filter.c | 32 ++++++++++++++++++--------------
4 files changed, 26 insertions(+), 19 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index af8601340dae..f2a03f232386 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -580,6 +580,7 @@ struct bpf_skb_data_end {
struct bpf_redirect_info {
u32 flags;
u32 tgt_index;
+ void *tgt_value;
struct bpf_map *map;
struct bpf_map *map_to_flush;
u32 kern_flags;
diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index bb5e380e2ef3..393f3d7ca819 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -146,9 +146,8 @@ struct _bpf_dtab_netdev {
#endif /* __DEVMAP_OBJ_TYPE */
#define devmap_ifindex(fwd, map) \
- (!fwd ? 0 : \
- ((map->map_type == BPF_MAP_TYPE_DEVMAP) ? \
- ((struct _bpf_dtab_netdev *)fwd)->dev->ifindex : 0))
+ ((map->map_type == BPF_MAP_TYPE_DEVMAP) ? \
+ ((struct _bpf_dtab_netdev *)fwd)->dev->ifindex : 0)
#define _trace_xdp_redirect_map(dev, xdp, fwd, map, idx) \
trace_xdp_redirect_map(dev, xdp, devmap_ifindex(fwd, map), \
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b077507efa3f..59f3fe61b2b0 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1568,8 +1568,11 @@ union bpf_attr {
* but this is only implemented for native XDP (with driver
* support) as of this writing).
*
- * All values for *flags* are reserved for future usage, and must
- * be left at zero.
+ * The lower two bits of *flags* are used as the return code if
+ * the map lookup fails. This is so that the return value can be
+ * one of the XDP program return codes up to XDP_TX, as chosen by
+ * the caller. Any higher bits in the *flags* argument must be
+ * unset.
*
* When used to redirect packets to net devices, this helper
* provides a high performance increase over **bpf_redirect**\ ().
diff --git a/net/core/filter.c b/net/core/filter.c
index 1d7f5f7f32cf..f9fcc8168e1c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3605,17 +3605,13 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
struct bpf_redirect_info *ri)
{
u32 index = ri->tgt_index;
- void *fwd = NULL;
+ void *fwd = ri->tgt_value;
int err;
ri->tgt_index = 0;
+ ri->tgt_value = NULL;
WRITE_ONCE(ri->map, NULL);
- fwd = __xdp_map_lookup_elem(map, index);
- if (unlikely(!fwd)) {
- err = -EINVAL;
- goto err;
- }
if (ri->map_to_flush && unlikely(ri->map_to_flush != map))
xdp_do_flush_map();
@@ -3652,18 +3648,13 @@ static int xdp_do_generic_redirect_map(struct net_device *dev,
{
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
u32 index = ri->tgt_index;
- void *fwd = NULL;
+ void *fwd = ri->tgt_value;
int err = 0;
ri->tgt_index = 0;
+ ri->tgt_value = NULL;
WRITE_ONCE(ri->map, NULL);
- fwd = __xdp_map_lookup_elem(map, index);
- if (unlikely(!fwd)) {
- err = -EINVAL;
- goto err;
- }
-
if (map->map_type == BPF_MAP_TYPE_DEVMAP) {
struct bpf_dtab_netdev *dst = fwd;
@@ -3732,6 +3723,7 @@ BPF_CALL_2(bpf_xdp_redirect, u32, ifindex, u64, flags)
ri->flags = flags;
ri->tgt_index = ifindex;
+ ri->tgt_value = NULL;
WRITE_ONCE(ri->map, NULL);
return XDP_REDIRECT;
@@ -3750,9 +3742,21 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
{
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
- if (unlikely(flags))
+ /* Lower bits of the flags are used as return code on lookup failure */
+ if (unlikely(flags > XDP_TX))
return XDP_ABORTED;
+ ri->tgt_value = __xdp_map_lookup_elem(map, ifindex);
+ if (unlikely(!ri->tgt_value)) {
+ /* If the lookup fails we want to clear out the state in the
+ * redirect_info struct completely, so that if an eBPF program
+ * performs multiple lookups, the last one always takes
+ * precedence.
+ */
+ WRITE_ONCE(ri->map, NULL);
+ return flags;
+ }
+
ri->flags = flags;
ri->tgt_index = ifindex;
WRITE_ONCE(ri->map, map);
^ permalink raw reply related
* [PATCH bpf-next v6 5/5] devmap: Allow map lookups from eBPF
From: Toke Høiland-Jørgensen @ 2019-06-28 9:12 UTC (permalink / raw)
To: netdev
Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
David Miller, Jonathan Lemon
In-Reply-To: <156171315462.9468.3367572649463706996.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
We don't currently allow lookups into a devmap from eBPF, because the map
lookup returns a pointer directly to the dev->ifindex, which shouldn't be
modifiable from eBPF.
However, being able to do lookups in devmaps is useful to know (e.g.)
whether forwarding to a specific interface is enabled. Currently, programs
work around this by keeping a shadow map of another type which indicates
whether a map index is valid.
Since we now have a flag to make maps read-only from the eBPF side, we can
simply lift the lookup restriction if we make sure this flag is always set.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
---
kernel/bpf/devmap.c | 5 +++++
kernel/bpf/verifier.c | 7 ++-----
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index a4dddc867cbf..d83cf8ccc872 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -89,6 +89,11 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
return ERR_PTR(-EINVAL);
+ /* Lookup returns a pointer straight to dev->ifindex, so make sure the
+ * verifier prevents writes from the BPF side
+ */
+ attr->map_flags |= BPF_F_RDONLY_PROG;
+
dtab = kzalloc(sizeof(*dtab), GFP_USER);
if (!dtab)
return ERR_PTR(-ENOMEM);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0e079b2298f8..51c327ee7d3f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3407,12 +3407,9 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
if (func_id != BPF_FUNC_get_local_storage)
goto error;
break;
- /* devmap returns a pointer to a live net_device ifindex that we cannot
- * allow to be modified from bpf side. So do not allow lookup elements
- * for now.
- */
case BPF_MAP_TYPE_DEVMAP:
- if (func_id != BPF_FUNC_redirect_map)
+ if (func_id != BPF_FUNC_redirect_map &&
+ func_id != BPF_FUNC_map_lookup_elem)
goto error;
break;
/* Restrict bpf side of cpumap and xskmap, open when use-cases
^ permalink raw reply related
* [PATCH bpf-next v6 3/5] devmap: Rename ifindex member in bpf_redirect_info
From: Toke Høiland-Jørgensen @ 2019-06-28 9:12 UTC (permalink / raw)
To: netdev
Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
David Miller, Jonathan Lemon
In-Reply-To: <156171315462.9468.3367572649463706996.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
The bpf_redirect_info struct has an 'ifindex' member which was named back
when the redirects could only target egress interfaces. Now that we can
also redirect to sockets and CPUs, this is a bit misleading, so rename the
member to tgt_index.
Reorder the struct members so we can have 'tgt_index' and 'tgt_value' next
to each other in a subsequent patch.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
include/linux/filter.h | 2 +-
net/core/filter.c | 26 +++++++++++++-------------
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 43b45d6db36d..af8601340dae 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -578,8 +578,8 @@ struct bpf_skb_data_end {
};
struct bpf_redirect_info {
- u32 ifindex;
u32 flags;
+ u32 tgt_index;
struct bpf_map *map;
struct bpf_map *map_to_flush;
u32 kern_flags;
diff --git a/net/core/filter.c b/net/core/filter.c
index 183bf4d8e301..1d7f5f7f32cf 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2158,8 +2158,8 @@ BPF_CALL_2(bpf_redirect, u32, ifindex, u64, flags)
if (unlikely(flags & ~(BPF_F_INGRESS)))
return TC_ACT_SHOT;
- ri->ifindex = ifindex;
ri->flags = flags;
+ ri->tgt_index = ifindex;
return TC_ACT_REDIRECT;
}
@@ -2169,8 +2169,8 @@ int skb_do_redirect(struct sk_buff *skb)
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
struct net_device *dev;
- dev = dev_get_by_index_rcu(dev_net(skb->dev), ri->ifindex);
- ri->ifindex = 0;
+ dev = dev_get_by_index_rcu(dev_net(skb->dev), ri->tgt_index);
+ ri->tgt_index = 0;
if (unlikely(!dev)) {
kfree_skb(skb);
return -EINVAL;
@@ -3488,11 +3488,11 @@ xdp_do_redirect_slow(struct net_device *dev, struct xdp_buff *xdp,
struct bpf_prog *xdp_prog, struct bpf_redirect_info *ri)
{
struct net_device *fwd;
- u32 index = ri->ifindex;
+ u32 index = ri->tgt_index;
int err;
fwd = dev_get_by_index_rcu(dev_net(dev), index);
- ri->ifindex = 0;
+ ri->tgt_index = 0;
if (unlikely(!fwd)) {
err = -EINVAL;
goto err;
@@ -3604,11 +3604,11 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
struct bpf_prog *xdp_prog, struct bpf_map *map,
struct bpf_redirect_info *ri)
{
- u32 index = ri->ifindex;
+ u32 index = ri->tgt_index;
void *fwd = NULL;
int err;
- ri->ifindex = 0;
+ ri->tgt_index = 0;
WRITE_ONCE(ri->map, NULL);
fwd = __xdp_map_lookup_elem(map, index);
@@ -3651,11 +3651,11 @@ static int xdp_do_generic_redirect_map(struct net_device *dev,
struct bpf_map *map)
{
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
- u32 index = ri->ifindex;
+ u32 index = ri->tgt_index;
void *fwd = NULL;
int err = 0;
- ri->ifindex = 0;
+ ri->tgt_index = 0;
WRITE_ONCE(ri->map, NULL);
fwd = __xdp_map_lookup_elem(map, index);
@@ -3695,14 +3695,14 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
{
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
struct bpf_map *map = READ_ONCE(ri->map);
- u32 index = ri->ifindex;
+ u32 index = ri->tgt_index;
struct net_device *fwd;
int err = 0;
if (map)
return xdp_do_generic_redirect_map(dev, skb, xdp, xdp_prog,
map);
- ri->ifindex = 0;
+ ri->tgt_index = 0;
fwd = dev_get_by_index_rcu(dev_net(dev), index);
if (unlikely(!fwd)) {
err = -EINVAL;
@@ -3730,8 +3730,8 @@ BPF_CALL_2(bpf_xdp_redirect, u32, ifindex, u64, flags)
if (unlikely(flags))
return XDP_ABORTED;
- ri->ifindex = ifindex;
ri->flags = flags;
+ ri->tgt_index = ifindex;
WRITE_ONCE(ri->map, NULL);
return XDP_REDIRECT;
@@ -3753,8 +3753,8 @@ BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
if (unlikely(flags))
return XDP_ABORTED;
- ri->ifindex = ifindex;
ri->flags = flags;
+ ri->tgt_index = ifindex;
WRITE_ONCE(ri->map, map);
return XDP_REDIRECT;
^ permalink raw reply related
* [PATCH bpf-next v6 1/5] xskmap: Move non-standard list manipulation to helper
From: Toke Høiland-Jørgensen @ 2019-06-28 9:12 UTC (permalink / raw)
To: netdev
Cc: Jesper Dangaard Brouer, Daniel Borkmann, Alexei Starovoitov,
David Miller, Jonathan Lemon
In-Reply-To: <156171315462.9468.3367572649463706996.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
Add a helper in list.h for the non-standard way of clearing a list that is
used in xskmap. This makes it easier to reuse it in the other map types,
and also makes sure this usage is not forgotten in any list refactorings in
the future.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
include/linux/list.h | 14 ++++++++++++++
kernel/bpf/xskmap.c | 3 +--
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/include/linux/list.h b/include/linux/list.h
index e951228db4b2..85c92555e31f 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -106,6 +106,20 @@ static inline void __list_del(struct list_head * prev, struct list_head * next)
WRITE_ONCE(prev->next, next);
}
+/*
+ * Delete a list entry and clear the 'prev' pointer.
+ *
+ * This is a special-purpose list clearing method used in the networking code
+ * for lists allocated as per-cpu, where we don't want to incur the extra
+ * WRITE_ONCE() overhead of a regular list_del_init(). The code that uses this
+ * needs to check the node 'prev' pointer instead of calling list_empty().
+ */
+static inline void __list_del_clearprev(struct list_head *entry)
+{
+ __list_del(entry->prev, entry->next);
+ entry->prev = NULL;
+}
+
/**
* list_del - deletes entry from list.
* @entry: the element to delete from the list.
diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
index ef7338cebd18..9bb96ace9fa1 100644
--- a/kernel/bpf/xskmap.c
+++ b/kernel/bpf/xskmap.c
@@ -145,8 +145,7 @@ void __xsk_map_flush(struct bpf_map *map)
list_for_each_entry_safe(xs, tmp, flush_list, flush_node) {
xsk_flush(xs);
- __list_del(xs->flush_node.prev, xs->flush_node.next);
- xs->flush_node.prev = NULL;
+ __list_del_clearprev(&xs->flush_node);
}
}
^ permalink raw reply related
* [PATCH 1/4 nf-next v2] netfilter:nf_flow_table: Refactor flow_offload_tuple to destination
From: wenxu @ 2019-06-28 9:48 UTC (permalink / raw)
To: pablo, fw; +Cc: netfilter-devel, netdev
From: wenxu <wenxu@ucloud.cn>
Add struct flow_offload_dst to support more offload method to replace
dst_cache which only work for route offload.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
include/net/netfilter/nf_flow_table.h | 12 ++++++++++--
net/netfilter/nf_flow_table_core.c | 22 +++++++++++-----------
net/netfilter/nf_flow_table_ip.c | 4 ++--
net/netfilter/nft_flow_offload.c | 12 ++++++------
4 files changed, 29 insertions(+), 21 deletions(-)
diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index d8c1879..d40d409 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -33,6 +33,10 @@ enum flow_offload_tuple_dir {
FLOW_OFFLOAD_DIR_MAX = IP_CT_DIR_MAX
};
+struct flow_offload_dst {
+ struct dst_entry *dst_cache;
+};
+
struct flow_offload_tuple {
union {
struct in_addr src_v4;
@@ -55,7 +59,7 @@ struct flow_offload_tuple {
u16 mtu;
- struct dst_entry *dst_cache;
+ struct flow_offload_dst dst;
};
struct flow_offload_tuple_rhash {
@@ -85,8 +89,12 @@ struct nf_flow_route {
} tuple[FLOW_OFFLOAD_DIR_MAX];
};
+struct nf_flow_dst {
+ struct nf_flow_route route;
+};
+
struct flow_offload *flow_offload_alloc(struct nf_conn *ct,
- struct nf_flow_route *route);
+ struct nf_flow_dst *flow_dst);
void flow_offload_free(struct flow_offload *flow);
int flow_offload_add(struct nf_flowtable *flow_table, struct flow_offload *flow);
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index e3d7972..7e0b5bd 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -24,13 +24,13 @@ struct flow_offload_entry {
static void
flow_offload_fill_dir(struct flow_offload *flow, struct nf_conn *ct,
- struct nf_flow_route *route,
+ struct nf_flow_dst *flow_dst,
enum flow_offload_tuple_dir dir)
{
struct flow_offload_tuple *ft = &flow->tuplehash[dir].tuple;
struct nf_conntrack_tuple *ctt = &ct->tuplehash[dir].tuple;
- struct dst_entry *other_dst = route->tuple[!dir].dst;
- struct dst_entry *dst = route->tuple[dir].dst;
+ struct dst_entry *other_dst = flow_dst->route.tuple[!dir].dst;
+ struct dst_entry *dst = flow_dst->route.tuple[dir].dst;
ft->dir = dir;
@@ -57,7 +57,7 @@ struct flow_offload_entry {
}
struct flow_offload *
-flow_offload_alloc(struct nf_conn *ct, struct nf_flow_route *route)
+flow_offload_alloc(struct nf_conn *ct, struct nf_flow_dst *flow_dst)
{
struct flow_offload_entry *entry;
struct flow_offload *flow;
@@ -72,16 +72,16 @@ struct flow_offload *
flow = &entry->flow;
- if (!dst_hold_safe(route->tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst))
+ if (!dst_hold_safe(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst))
goto err_dst_cache_original;
- if (!dst_hold_safe(route->tuple[FLOW_OFFLOAD_DIR_REPLY].dst))
+ if (!dst_hold_safe(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_REPLY].dst))
goto err_dst_cache_reply;
entry->ct = ct;
- flow_offload_fill_dir(flow, ct, route, FLOW_OFFLOAD_DIR_ORIGINAL);
- flow_offload_fill_dir(flow, ct, route, FLOW_OFFLOAD_DIR_REPLY);
+ flow_offload_fill_dir(flow, ct, flow_dst, FLOW_OFFLOAD_DIR_ORIGINAL);
+ flow_offload_fill_dir(flow, ct, flow_dst, FLOW_OFFLOAD_DIR_REPLY);
if (ct->status & IPS_SRC_NAT)
flow->flags |= FLOW_OFFLOAD_SNAT;
@@ -91,7 +91,7 @@ struct flow_offload *
return flow;
err_dst_cache_reply:
- dst_release(route->tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst);
+ dst_release(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst);
err_dst_cache_original:
kfree(entry);
err_ct_refcnt:
@@ -139,8 +139,8 @@ void flow_offload_free(struct flow_offload *flow)
{
struct flow_offload_entry *e;
- dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst_cache);
- dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst_cache);
+ dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.dst_cache);
+ dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst.dst_cache);
e = container_of(flow, struct flow_offload_entry, flow);
if (flow->flags & FLOW_OFFLOAD_DYING)
nf_ct_delete(e->ct, 0, 0);
diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index 2413174..0016bb8 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -241,7 +241,7 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
dir = tuplehash->tuple.dir;
flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
- rt = (struct rtable *)flow->tuplehash[dir].tuple.dst_cache;
+ rt = (struct rtable *)flow->tuplehash[dir].tuple.dst.dst_cache;
outdev = rt->dst.dev;
if (unlikely(nf_flow_exceeds_mtu(skb, flow->tuplehash[dir].tuple.mtu)))
@@ -457,7 +457,7 @@ static int nf_flow_tuple_ipv6(struct sk_buff *skb, const struct net_device *dev,
dir = tuplehash->tuple.dir;
flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
- rt = (struct rt6_info *)flow->tuplehash[dir].tuple.dst_cache;
+ rt = (struct rt6_info *)flow->tuplehash[dir].tuple.dst.dst_cache;
outdev = rt->dst.dev;
if (unlikely(nf_flow_exceeds_mtu(skb, flow->tuplehash[dir].tuple.mtu)))
diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index aa5f571..261c565 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -70,10 +70,10 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
struct nft_regs *regs,
const struct nft_pktinfo *pkt)
{
- struct nft_flow_offload *priv = nft_expr_priv(expr);
struct nf_flowtable *flowtable = &priv->flowtable->data;
+ struct nft_flow_offload *priv = nft_expr_priv(expr);
enum ip_conntrack_info ctinfo;
- struct nf_flow_route route;
+ struct nf_flow_dst flow_dst;
struct flow_offload *flow;
enum ip_conntrack_dir dir;
bool is_tcp = false;
@@ -108,10 +108,10 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
goto out;
dir = CTINFO2DIR(ctinfo);
- if (nft_flow_route(pkt, ct, &route, dir) < 0)
+ if (nft_flow_route(pkt, ct, &flow_dst.route, dir) < 0)
goto err_flow_route;
- flow = flow_offload_alloc(ct, &route);
+ flow = flow_offload_alloc(ct, &flow_dst);
if (!flow)
goto err_flow_alloc;
@@ -124,13 +124,13 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
if (ret < 0)
goto err_flow_add;
- dst_release(route.tuple[!dir].dst);
+ dst_release(flow_dst.route.tuple[!dir].dst);
return;
err_flow_add:
flow_offload_free(flow);
err_flow_alloc:
- dst_release(route.tuple[!dir].dst);
+ dst_release(flow_dst.route.tuple[!dir].dst);
err_flow_route:
clear_bit(IPS_OFFLOAD_BIT, &ct->status);
out:
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/4 nf-next v2] netfilter:nf_flow_table: Separate inet operation to single function
From: wenxu @ 2019-06-28 9:48 UTC (permalink / raw)
To: pablo, fw; +Cc: netfilter-devel, netdev
In-Reply-To: <1561715326-3945-1-git-send-email-wenxu@ucloud.cn>
From: wenxu <wenxu@ucloud.cn>
This patch separate the inet family operation to single function.
Prepare for supporting the bridge family.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
net/netfilter/nf_flow_table_core.c | 52 ++++++++++++++++++++++++++++----------
net/netfilter/nf_flow_table_ip.c | 34 +++++++++++++++----------
net/netfilter/nft_flow_offload.c | 2 +-
3 files changed, 60 insertions(+), 28 deletions(-)
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 7e0b5bd..2bec409 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -22,6 +22,20 @@ struct flow_offload_entry {
static DEFINE_MUTEX(flowtable_lock);
static LIST_HEAD(flowtables);
+static struct dst_entry *
+flow_offload_fill_inet_dst(struct flow_offload_tuple *ft,
+ struct nf_flow_route *route,
+ enum flow_offload_tuple_dir dir)
+{
+ struct dst_entry *other_dst = route->tuple[!dir].dst;
+ struct dst_entry *dst = route->tuple[dir].dst;
+
+ ft->iifidx = other_dst->dev->ifindex;
+ ft->dst.dst_cache = dst;
+
+ return dst;
+}
+
static void
flow_offload_fill_dir(struct flow_offload *flow, struct nf_conn *ct,
struct nf_flow_dst *flow_dst,
@@ -29,9 +43,9 @@ struct flow_offload_entry {
{
struct flow_offload_tuple *ft = &flow->tuplehash[dir].tuple;
struct nf_conntrack_tuple *ctt = &ct->tuplehash[dir].tuple;
- struct dst_entry *other_dst = flow_dst->route.tuple[!dir].dst;
- struct dst_entry *dst = flow_dst->route.tuple[dir].dst;
+ struct dst_entry *dst;
+ dst = flow_offload_fill_inet_dst(ft, &flow_dst->route, dir);
ft->dir = dir;
switch (ctt->src.l3num) {
@@ -51,9 +65,19 @@ struct flow_offload_entry {
ft->l4proto = ctt->dst.protonum;
ft->src_port = ctt->src.u.tcp.port;
ft->dst_port = ctt->dst.u.tcp.port;
+}
- ft->iifidx = other_dst->dev->ifindex;
- ft->dst_cache = dst;
+static int flow_offload_dst_hold(struct nf_flow_dst *flow_dst)
+{
+ if (!dst_hold_safe(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst))
+ return -1;
+
+ if (!dst_hold_safe(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_REPLY].dst)) {
+ dst_release(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst);
+ return -1;
+ }
+
+ return 0;
}
struct flow_offload *
@@ -72,11 +96,8 @@ struct flow_offload *
flow = &entry->flow;
- if (!dst_hold_safe(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst))
- goto err_dst_cache_original;
-
- if (!dst_hold_safe(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_REPLY].dst))
- goto err_dst_cache_reply;
+ if (flow_offload_dst_hold(flow_dst))
+ goto err_dst_cache;
entry->ct = ct;
@@ -90,9 +111,7 @@ struct flow_offload *
return flow;
-err_dst_cache_reply:
- dst_release(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst);
-err_dst_cache_original:
+err_dst_cache:
kfree(entry);
err_ct_refcnt:
nf_ct_put(ct);
@@ -135,12 +154,17 @@ static void flow_offload_fixup_ct_state(struct nf_conn *ct)
ct->timeout = nfct_time_stamp + timeout;
}
+static void flow_offload_dst_release(struct flow_offload *flow)
+{
+ dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.dst_cache);
+ dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst.dst_cache);
+}
+
void flow_offload_free(struct flow_offload *flow)
{
struct flow_offload_entry *e;
- dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.dst_cache);
- dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst.dst_cache);
+ flow_offload_dst_release(flow);
e = container_of(flow, struct flow_offload_entry, flow);
if (flow->flags & FLOW_OFFLOAD_DYING)
nf_ct_delete(e->ct, 0, 0);
diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index 0016bb8..24263e2 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -214,6 +214,25 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
return true;
}
+static void nf_flow_inet_xmit(struct flow_offload *flow, struct sk_buff *skb,
+ enum flow_offload_tuple_dir dir)
+{
+ struct net_device *outdev;
+ struct rtable *rt;
+ struct iphdr *iph;
+ __be32 nexthop;
+
+ rt = (struct rtable *)flow->tuplehash[dir].tuple.dst.dst_cache;
+ outdev = rt->dst.dev;
+ iph = ip_hdr(skb);
+ ip_decrease_ttl(iph);
+
+ skb->dev = outdev;
+ nexthop = rt_nexthop(rt, flow->tuplehash[!dir].tuple.src_v4.s_addr);
+ skb_dst_set_noref(skb, &rt->dst);
+ neigh_xmit(NEIGH_ARP_TABLE, outdev, &nexthop, skb);
+}
+
unsigned int
nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
const struct nf_hook_state *state)
@@ -223,11 +242,7 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
struct flow_offload_tuple tuple = {};
enum flow_offload_tuple_dir dir;
struct flow_offload *flow;
- struct net_device *outdev;
- struct rtable *rt;
unsigned int thoff;
- struct iphdr *iph;
- __be32 nexthop;
if (skb->protocol != htons(ETH_P_IP))
return NF_ACCEPT;
@@ -241,13 +256,11 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
dir = tuplehash->tuple.dir;
flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
- rt = (struct rtable *)flow->tuplehash[dir].tuple.dst.dst_cache;
- outdev = rt->dst.dev;
if (unlikely(nf_flow_exceeds_mtu(skb, flow->tuplehash[dir].tuple.mtu)))
return NF_ACCEPT;
- if (skb_try_make_writable(skb, sizeof(*iph)))
+ if (skb_try_make_writable(skb, sizeof(struct iphdr)))
return NF_DROP;
thoff = ip_hdr(skb)->ihl * 4;
@@ -258,13 +271,8 @@ static bool nf_flow_exceeds_mtu(const struct sk_buff *skb, unsigned int mtu)
return NF_DROP;
flow->timeout = (u32)jiffies + NF_FLOW_TIMEOUT;
- iph = ip_hdr(skb);
- ip_decrease_ttl(iph);
- skb->dev = outdev;
- nexthop = rt_nexthop(rt, flow->tuplehash[!dir].tuple.src_v4.s_addr);
- skb_dst_set_noref(skb, &rt->dst);
- neigh_xmit(NEIGH_ARP_TABLE, outdev, &nexthop, skb);
+ nf_flow_inet_xmit(flow, skb, dir);
return NF_STOLEN;
}
diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index 261c565..4af94ce 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -70,8 +70,8 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
struct nft_regs *regs,
const struct nft_pktinfo *pkt)
{
- struct nf_flowtable *flowtable = &priv->flowtable->data;
struct nft_flow_offload *priv = nft_expr_priv(expr);
+ struct nf_flowtable *flowtable = &priv->flowtable->data;
enum ip_conntrack_info ctinfo;
struct nf_flow_dst flow_dst;
struct flow_offload *flow;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 3/4 nf-next v2] netfilter:nf_flow_table: Support bridge family flow offload
From: wenxu @ 2019-06-28 9:48 UTC (permalink / raw)
To: pablo, fw; +Cc: netfilter-devel, netdev
In-Reply-To: <1561715326-3945-1-git-send-email-wenxu@ucloud.cn>
From: wenxu <wenxu@ucloud.cn>
With nf_conntrack_bridge function. The bridge family can do
conntrack it self. The flow offload function based on the
conntrack. So the flow in the bridge wih conntrack can be
offloaded.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
include/net/netfilter/nf_flow_table.h | 31 +++++++-
net/bridge/br_vlan.c | 1 +
net/netfilter/nf_flow_table_core.c | 58 ++++++++++++---
net/netfilter/nf_flow_table_ip.c | 43 +++++++++++-
net/netfilter/nft_flow_offload.c | 129 ++++++++++++++++++++++++++++++++--
5 files changed, 243 insertions(+), 19 deletions(-)
diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index d40d409..dcf197a 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -33,8 +33,23 @@ enum flow_offload_tuple_dir {
FLOW_OFFLOAD_DIR_MAX = IP_CT_DIR_MAX
};
+enum flow_offload_tuple_type {
+ FLOW_OFFLOAD_TYPE_INET,
+ FLOW_OFFLOAD_TYPE_BRIDGE,
+};
+
+struct dst_br_port {
+ struct net_device *dev;
+ u16 dst_vlan_tag;
+ u16 vlan_proto;
+};
+
struct flow_offload_dst {
- struct dst_entry *dst_cache;
+ enum flow_offload_tuple_type type;
+ union {
+ struct dst_entry *dst_cache;
+ struct dst_br_port dst_port;
+ };
};
struct flow_offload_tuple {
@@ -52,6 +67,7 @@ struct flow_offload_tuple {
};
int iifidx;
+ u16 vlan_tag;
u8 l3proto;
u8 l4proto;
@@ -89,8 +105,19 @@ struct nf_flow_route {
} tuple[FLOW_OFFLOAD_DIR_MAX];
};
+struct nf_flow_forward {
+ struct {
+ struct dst_br_port dst_port;
+ u16 vlan_tag;
+ } tuple[FLOW_OFFLOAD_DIR_MAX];
+};
+
struct nf_flow_dst {
- struct nf_flow_route route;
+ enum flow_offload_tuple_type type;
+ union {
+ struct nf_flow_route route;
+ struct nf_flow_forward forward;
+ };
};
struct flow_offload *flow_offload_alloc(struct nf_conn *ct,
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index f47f526..bcb7ce4 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -729,6 +729,7 @@ struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 vid)
return br_vlan_lookup(&vg->vlan_hash, vid);
}
+EXPORT_SYMBOL_GPL(br_vlan_find);
/* Must be protected by RTNL. */
static void recalculate_group_addr(struct net_bridge *br)
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 2bec409..08c1ca4 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -36,6 +36,21 @@ struct flow_offload_entry {
return dst;
}
+static struct dst_br_port *
+flow_offload_fill_bridge_dst(struct flow_offload_tuple *ft,
+ struct nf_flow_forward *forward,
+ enum flow_offload_tuple_dir dir)
+{
+ struct dst_br_port other_dst_port = forward->tuple[!dir].dst_port;
+ struct dst_br_port dst_port = forward->tuple[dir].dst_port;
+
+ ft->iifidx = other_dst_port.dev->ifindex;
+ ft->dst.dst_port = dst_port;
+ ft->vlan_tag = forward->tuple[dir].vlan_tag;
+
+ return &ft->dst.dst_port;
+}
+
static void
flow_offload_fill_dir(struct flow_offload *flow, struct nf_conn *ct,
struct nf_flow_dst *flow_dst,
@@ -43,16 +58,29 @@ struct flow_offload_entry {
{
struct flow_offload_tuple *ft = &flow->tuplehash[dir].tuple;
struct nf_conntrack_tuple *ctt = &ct->tuplehash[dir].tuple;
+ struct dst_br_port *dst_port;
struct dst_entry *dst;
- dst = flow_offload_fill_inet_dst(ft, &flow_dst->route, dir);
+ switch (flow_dst->type) {
+ case FLOW_OFFLOAD_TYPE_INET:
+ dst = flow_offload_fill_inet_dst(ft, &flow_dst->route, dir);
+ break;
+ case FLOW_OFFLOAD_TYPE_BRIDGE:
+ dst_port = flow_offload_fill_bridge_dst(ft, &flow_dst->forward, dir);
+ break;
+ }
+
+ ft->dst.type = flow_dst->type;
ft->dir = dir;
switch (ctt->src.l3num) {
case NFPROTO_IPV4:
ft->src_v4 = ctt->src.u3.in;
ft->dst_v4 = ctt->dst.u3.in;
- ft->mtu = ip_dst_mtu_maybe_forward(dst, true);
+ if (flow_dst->type == FLOW_OFFLOAD_TYPE_INET)
+ ft->mtu = ip_dst_mtu_maybe_forward(dst, true);
+ else
+ ft->mtu = dst_port->dev->mtu;
break;
case NFPROTO_IPV6:
ft->src_v6 = ctt->src.u3.in6;
@@ -67,13 +95,13 @@ struct flow_offload_entry {
ft->dst_port = ctt->dst.u.tcp.port;
}
-static int flow_offload_dst_hold(struct nf_flow_dst *flow_dst)
+static int flow_offload_dst_hold(struct nf_flow_route *route)
{
- if (!dst_hold_safe(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst))
+ if (!dst_hold_safe(route->tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst))
return -1;
- if (!dst_hold_safe(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_REPLY].dst)) {
- dst_release(flow_dst->route.tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst);
+ if (!dst_hold_safe(route->tuple[FLOW_OFFLOAD_DIR_REPLY].dst)) {
+ dst_release(route->tuple[FLOW_OFFLOAD_DIR_ORIGINAL].dst);
return -1;
}
@@ -96,7 +124,8 @@ struct flow_offload *
flow = &entry->flow;
- if (flow_offload_dst_hold(flow_dst))
+ if (flow_dst->type == FLOW_OFFLOAD_TYPE_INET &&
+ flow_offload_dst_hold(&flow_dst->route))
goto err_dst_cache;
entry->ct = ct;
@@ -156,8 +185,19 @@ static void flow_offload_fixup_ct_state(struct nf_conn *ct)
static void flow_offload_dst_release(struct flow_offload *flow)
{
- dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.dst_cache);
- dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst.dst_cache);
+ enum flow_offload_tuple_type type = flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.type;
+
+ switch (type) {
+ case FLOW_OFFLOAD_TYPE_INET:
+ dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.dst_cache);
+ dst_release(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst.dst_cache);
+ break;
+
+ case FLOW_OFFLOAD_TYPE_BRIDGE:
+ dev_put(flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.dst.dst_port.dev);
+ dev_put(flow->tuplehash[FLOW_OFFLOAD_DIR_REPLY].tuple.dst.dst_port.dev);
+ break;
+ }
}
void flow_offload_free(struct flow_offload *flow)
diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
index 24263e2..490b4be 100644
--- a/net/netfilter/nf_flow_table_ip.c
+++ b/net/netfilter/nf_flow_table_ip.c
@@ -233,12 +233,40 @@ static void nf_flow_inet_xmit(struct flow_offload *flow, struct sk_buff *skb,
neigh_xmit(NEIGH_ARP_TABLE, outdev, &nexthop, skb);
}
+static void nf_flow_bridge_xmit(struct flow_offload *flow, struct sk_buff *skb,
+ enum flow_offload_tuple_dir dir)
+{
+ struct net_device *outdev;
+ u16 vlan_tag, vlan_proto;
+
+ vlan_tag = flow->tuplehash[dir].tuple.dst.dst_port.dst_vlan_tag;
+ vlan_proto = flow->tuplehash[dir].tuple.dst.dst_port.vlan_proto;
+ outdev = flow->tuplehash[dir].tuple.dst.dst_port.dev;
+ skb->dev = outdev;
+
+ if (vlan_tag)
+ __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tag);
+ else
+ __vlan_hwaccel_clear_tag(skb);
+
+ skb_push(skb, ETH_HLEN);
+ if (!is_skb_forwardable(skb->dev, skb))
+ goto drop;
+
+ dev_queue_xmit(skb);
+ return;
+
+drop:
+ kfree_skb(skb);
+}
+
unsigned int
nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
const struct nf_hook_state *state)
{
struct flow_offload_tuple_rhash *tuplehash;
struct nf_flowtable *flow_table = priv;
+ int family = flow_table->type->family;
struct flow_offload_tuple tuple = {};
enum flow_offload_tuple_dir dir;
struct flow_offload *flow;
@@ -247,9 +275,15 @@ static void nf_flow_inet_xmit(struct flow_offload *flow, struct sk_buff *skb,
if (skb->protocol != htons(ETH_P_IP))
return NF_ACCEPT;
+ if (family != NFPROTO_BRIDGE && family != NFPROTO_IPV4)
+ return NF_ACCEPT;
+
if (nf_flow_tuple_ip(skb, state->in, &tuple) < 0)
return NF_ACCEPT;
+ if (family == NFPROTO_BRIDGE && skb_vlan_tag_present(skb))
+ tuple.vlan_tag = skb_vlan_tag_get_id(skb);
+
tuplehash = flow_offload_lookup(flow_table, &tuple);
if (tuplehash == NULL)
return NF_ACCEPT;
@@ -272,7 +306,14 @@ static void nf_flow_inet_xmit(struct flow_offload *flow, struct sk_buff *skb,
flow->timeout = (u32)jiffies + NF_FLOW_TIMEOUT;
- nf_flow_inet_xmit(flow, skb, dir);
+ switch (family) {
+ case NFPROTO_IPV4:
+ nf_flow_inet_xmit(flow, skb, dir);
+ break;
+ case NFPROTO_BRIDGE:
+ nf_flow_bridge_xmit(flow, skb, dir);
+ break;
+ }
return NF_STOLEN;
}
diff --git a/net/netfilter/nft_flow_offload.c b/net/netfilter/nft_flow_offload.c
index 4af94ce..0cdadd4 100644
--- a/net/netfilter/nft_flow_offload.c
+++ b/net/netfilter/nft_flow_offload.c
@@ -14,6 +14,10 @@
#include <linux/netfilter/nf_conntrack_common.h>
#include <net/netfilter/nf_flow_table.h>
+#ifdef CONFIG_NF_TABLES_BRIDGE
+#include "../bridge/br_private.h"
+#endif
+
struct nft_flow_offload {
struct nft_flowtable *flowtable;
};
@@ -49,23 +53,130 @@ static int nft_flow_route(const struct nft_pktinfo *pkt,
return 0;
}
+static int nft_flow_forward(const struct nft_pktinfo *pkt,
+ const struct nf_conn *ct,
+ struct nf_flow_forward *forward,
+ enum ip_conntrack_dir dir)
+{
+#ifdef CONFIG_NF_TABLES_BRIDGE
+ struct net_bridge_vlan_group *vg;
+ const struct net_bridge_port *p;
+ u16 vlan_proto = 0;
+ u16 vid = 0;
+
+ if (skb_vlan_tag_present(pkt->skb)) {
+ vid = skb_vlan_tag_get_id(pkt->skb);
+ vlan_proto = pkt->skb->vlan_proto;
+ }
+
+ forward->tuple[dir].dst_port.dst_vlan_tag = vid;
+ forward->tuple[dir].dst_port.vlan_proto = vlan_proto;
+ forward->tuple[!dir].vlan_tag = vid;
+ forward->tuple[dir].dst_port.dev = dev_get_by_index(dev_net(nft_out(pkt)),
+ nft_out(pkt)->ifindex);
+ forward->tuple[!dir].dst_port.dev = dev_get_by_index(dev_net(nft_in(pkt)),
+ nft_in(pkt)->ifindex);
+
+ p = br_port_get_rtnl_rcu(nft_out(pkt));
+ if (!p)
+ goto err;
+ if (!br_opt_get(p->br, BROPT_VLAN_ENABLED))
+ goto out;
+
+ if (!vid) {
+ vg = nbp_vlan_group_rcu(p);
+ vid = br_get_pvid(vg);
+ }
+
+ if (vid) {
+ struct net_bridge_vlan *vlan;
+
+ p = br_port_get_rtnl_rcu(nft_in(pkt));
+ if (!p)
+ goto err;
+
+ vlan_proto = p->br->vlan_proto;
+ vg = nbp_vlan_group_rcu(p);
+ vlan = br_vlan_find(vg, vid);
+ if (vlan && vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED) {
+ vid = 0;
+ vlan_proto = 0;
+ }
+ }
+
+out:
+ forward->tuple[!dir].dst_port.vlan_proto = vlan_proto;
+ forward->tuple[!dir].dst_port.dst_vlan_tag = vid;
+ forward->tuple[dir].vlan_tag = vid;
+
+ return 0;
+
+err:
+ dev_put(forward->tuple[dir].dst_port.dev);
+ dev_put(forward->tuple[!dir].dst_port.dev);
+#endif
+ return -ENOENT;
+}
+
static bool nft_flow_offload_skip(struct sk_buff *skb, int family)
{
if (skb_sec_path(skb))
return true;
- if (family == NFPROTO_IPV4) {
+ switch (family) {
+ case NFPROTO_IPV4: {
const struct ip_options *opt;
opt = &(IPCB(skb)->opt);
if (unlikely(opt->optlen))
return true;
+ break;
+ }
+ case NFPROTO_BRIDGE: {
+ const struct iphdr *iph;
+
+ if (skb->protocol != htons(ETH_P_IP))
+ return true;
+
+ iph = ip_hdr(skb);
+ if (iph->ihl > 5)
+ return true;
+ break;
+ }
}
return false;
}
+static void flow_offload_release_dst(struct nf_flow_dst *flow_dst,
+ enum ip_conntrack_dir dir)
+{
+ if (flow_dst->type == FLOW_OFFLOAD_TYPE_BRIDGE) {
+ dev_put(flow_dst->forward.tuple[dir].dst_port.dev);
+ dev_put(flow_dst->forward.tuple[!dir].dst_port.dev);
+ } else {
+ dst_release(flow_dst->route.tuple[!dir].dst);
+ }
+}
+
+static int flow_offload_get_dst(const struct nft_pktinfo *pkt, struct nf_conn *ct,
+ enum ip_conntrack_dir dir, int family,
+ struct nf_flow_dst *flow_dst)
+{
+ if (family == NFPROTO_BRIDGE) {
+ flow_dst->type = FLOW_OFFLOAD_TYPE_BRIDGE;
+ if (nft_flow_forward(pkt, ct, &flow_dst->forward, dir) < 0)
+ return -1;
+ } else {
+ flow_dst->type = FLOW_OFFLOAD_TYPE_INET;
+ if (nft_flow_route(pkt, ct, &flow_dst->route, dir) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
static void nft_flow_offload_eval(const struct nft_expr *expr,
struct nft_regs *regs,
const struct nft_pktinfo *pkt)
@@ -76,11 +187,12 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
struct nf_flow_dst flow_dst;
struct flow_offload *flow;
enum ip_conntrack_dir dir;
+ int family = nft_pf(pkt);
bool is_tcp = false;
struct nf_conn *ct;
int ret;
- if (nft_flow_offload_skip(pkt->skb, nft_pf(pkt)))
+ if (nft_flow_offload_skip(pkt->skb, family))
goto out;
ct = nf_ct_get(pkt->skb, &ctinfo);
@@ -108,8 +220,9 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
goto out;
dir = CTINFO2DIR(ctinfo);
- if (nft_flow_route(pkt, ct, &flow_dst.route, dir) < 0)
- goto err_flow_route;
+
+ if (flow_offload_get_dst(pkt, ct, dir, family, &flow_dst) < 0)
+ goto err_flow_dst;
flow = flow_offload_alloc(ct, &flow_dst);
if (!flow)
@@ -124,14 +237,16 @@ static void nft_flow_offload_eval(const struct nft_expr *expr,
if (ret < 0)
goto err_flow_add;
- dst_release(flow_dst.route.tuple[!dir].dst);
+ if (family != NFPROTO_BRIDGE)
+ dst_release(flow_dst.route.tuple[!dir].dst);
+
return;
err_flow_add:
flow_offload_free(flow);
err_flow_alloc:
- dst_release(flow_dst.route.tuple[!dir].dst);
-err_flow_route:
+ flow_offload_release_dst(&flow_dst, dir);
+err_flow_dst:
clear_bit(IPS_OFFLOAD_BIT, &ct->status);
out:
regs->verdict.code = NFT_BREAK;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 4/4 nf-next v2] netfilter: Flow table support for the bridge family
From: wenxu @ 2019-06-28 9:48 UTC (permalink / raw)
To: pablo, fw; +Cc: netfilter-devel, netdev
In-Reply-To: <1561715326-3945-1-git-send-email-wenxu@ucloud.cn>
From: wenxu <wenxu@ucloud.cn>
This patch adds the bridge flow table type, that implements the datapath
flow table to forward IPv4 traffic through bridge.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
net/bridge/netfilter/Kconfig | 8 +++++
net/bridge/netfilter/Makefile | 1 +
net/bridge/netfilter/nf_flow_table_bridge.c | 46 +++++++++++++++++++++++++++++
3 files changed, 55 insertions(+)
create mode 100644 net/bridge/netfilter/nf_flow_table_bridge.c
diff --git a/net/bridge/netfilter/Kconfig b/net/bridge/netfilter/Kconfig
index f4fb0b9..cba5f71 100644
--- a/net/bridge/netfilter/Kconfig
+++ b/net/bridge/netfilter/Kconfig
@@ -33,6 +33,14 @@ config NF_CONNTRACK_BRIDGE
To compile it as a module, choose M here. If unsure, say N.
+config NF_FLOW_TABLE_BRIDGE
+ tristate "Netfilter flow table bridge module"
+ depends on NF_FLOW_TABLE && NF_CONNTRACK_BRIDGE
+ help
+ This option adds the flow table bridge support.
+
+ To compile it as a module, choose M here.
+
endif # NF_TABLES_BRIDGE
menuconfig BRIDGE_NF_EBTABLES
diff --git a/net/bridge/netfilter/Makefile b/net/bridge/netfilter/Makefile
index 9d77673..deb81e6 100644
--- a/net/bridge/netfilter/Makefile
+++ b/net/bridge/netfilter/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_NFT_BRIDGE_REJECT) += nft_reject_bridge.o
# connection tracking
obj-$(CONFIG_NF_CONNTRACK_BRIDGE) += nf_conntrack_bridge.o
+obj-$(CONFIG_NF_FLOW_TABLE_BRIDGE) += nf_flow_table_bridge.o
# packet logging
obj-$(CONFIG_NF_LOG_BRIDGE) += nf_log_bridge.o
diff --git a/net/bridge/netfilter/nf_flow_table_bridge.c b/net/bridge/netfilter/nf_flow_table_bridge.c
new file mode 100644
index 0000000..3a65b44
--- /dev/null
+++ b/net/bridge/netfilter/nf_flow_table_bridge.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/netfilter.h>
+#include <net/netfilter/nf_flow_table.h>
+#include <net/netfilter/nf_tables.h>
+
+static unsigned int
+nf_flow_offload_bridge_hook(void *priv, struct sk_buff *skb,
+ const struct nf_hook_state *state)
+{
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ return nf_flow_offload_ip_hook(priv, skb, state);
+ }
+
+ return NF_ACCEPT;
+}
+
+static struct nf_flowtable_type flowtable_bridge = {
+ .family = NFPROTO_BRIDGE,
+ .init = nf_flow_table_init,
+ .free = nf_flow_table_free,
+ .hook = nf_flow_offload_bridge_hook,
+ .owner = THIS_MODULE,
+};
+
+static int __init nf_flow_bridge_module_init(void)
+{
+ nft_register_flowtable_type(&flowtable_bridge);
+
+ return 0;
+}
+
+static void __exit nf_flow_bridge_module_exit(void)
+{
+ nft_unregister_flowtable_type(&flowtable_bridge);
+}
+
+module_init(nf_flow_bridge_module_init);
+module_exit(nf_flow_bridge_module_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("wenxu <wenxu@ucloud.cn>");
+MODULE_ALIAS_NF_FLOWTABLE(AF_BRIDGE);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 2/3 nf-next] netfilter:nf_flow_table: Support bridge type flow offload
From: wenxu @ 2019-06-28 9:51 UTC (permalink / raw)
To: Florian Westphal; +Cc: Pablo Neira Ayuso, netfilter-devel, netdev
In-Reply-To: <20190628060617.az2quv4bodrenuli@breakpoint.cc>
On 6/28/2019 2:06 PM, Florian Westphal wrote:
> wenxu <wenxu@ucloud.cn> wrote:
>> ns21 iperf to 10.0.0.8 with dport 22 in ns22
>> first time with OFFLOAD enable
>>
>> nft add flowtable bridge firewall fb2 { hook ingress priority 0 \; devices = { veth21, veth22 } \; }
>> nft add chain bridge firewall ftb-all {type filter hook forward priority 0 \; policy accept \; }
>> nft add rule bridge firewall ftb-all counter ct zone 2 ip protocol tcp flow offload @fb2
>>
>> # iperf -c 10.0.0.8 -p 22 -t 60 -i2
> [..]
>> [ 3] 0.0-60.0 sec 353 GBytes 50.5 Gbits/sec
>>
>> The second time on any offload:
>> # iperf -c 10.0.0.8 -p 22 -t 60 -i2
>> [ 3] 0.0-60.0 sec 271 GBytes 38.8 Gbits/sec
> Wow, this is pretty impressive. Do you have numbers without
> offload and no connection tracking?
There is no other connection on the bridge in zone 2
>
> Is this with CONFIG_RETPOLINE=y (just curious)?
Yes, it is enable.
^ permalink raw reply
* [PATCH iproute2-next v4 1/2] ipaddress: correctly print a VF hw address in the IPoIB case
From: Denis Kirjanov @ 2019-06-28 9:54 UTC (permalink / raw)
To: stephen, dsahern; +Cc: netdev, linux-rdma, dledford, mkubecek, Denis Kirjanov
Current code assumes that we print ethernet mac and
that doesn't work in the IPoIB case with SRIOV-enabled hardware
Before:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state
disable,
trust off, query_rss off
...
After:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
checking off, link-state disable, trust off, query_rss off
v1->v2: updated kernel headers to uapi commit
v2->v3: fixed alignment
v3->v4: aligned print statements as used through the source
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
ip/ipaddress.c | 40 +++++++++++++++++++++++++++++++++++-----
1 file changed, 35 insertions(+), 5 deletions(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index b504200b..741b8667 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -26,6 +26,7 @@
#include <linux/netdevice.h>
#include <linux/if_arp.h>
+#include <linux/if_infiniband.h>
#include <linux/sockios.h>
#include <linux/net_namespace.h>
@@ -349,9 +350,10 @@ static void print_af_spec(FILE *fp, struct rtattr *af_spec_attr)
static void print_vf_stats64(FILE *fp, struct rtattr *vfstats);
-static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
+static void print_vfinfo(struct ifinfomsg *ifi, FILE *fp, struct rtattr *vfinfo)
{
struct ifla_vf_mac *vf_mac;
+ struct ifla_vf_broadcast *vf_broadcast;
struct ifla_vf_tx_rate *vf_tx_rate;
struct rtattr *vf[IFLA_VF_MAX + 1] = {};
@@ -365,13 +367,41 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
parse_rtattr_nested(vf, IFLA_VF_MAX, vfinfo);
vf_mac = RTA_DATA(vf[IFLA_VF_MAC]);
+ vf_broadcast = RTA_DATA(vf[IFLA_VF_BROADCAST]);
vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
print_string(PRINT_FP, NULL, "%s ", _SL_);
print_int(PRINT_ANY, "vf", "vf %d ", vf_mac->vf);
- print_string(PRINT_ANY, "mac", "MAC %s",
- ll_addr_n2a((unsigned char *) &vf_mac->mac,
- ETH_ALEN, 0, b1, sizeof(b1)));
+
+ print_string(PRINT_ANY,
+ "link_type",
+ " link/%s ",
+ ll_type_n2a(ifi->ifi_type, b1, sizeof(b1)));
+
+ print_color_string(PRINT_ANY, COLOR_MAC,
+ "address", "%s",
+ ll_addr_n2a((unsigned char *) &vf_mac->mac,
+ ifi->ifi_type == ARPHRD_ETHER ?
+ ETH_ALEN : INFINIBAND_ALEN,
+ ifi->ifi_type,
+ b1, sizeof(b1)));
+
+ if (vf[IFLA_VF_BROADCAST]) {
+ if (ifi->ifi_flags&IFF_POINTOPOINT) {
+ print_string(PRINT_FP, NULL, " peer ", NULL);
+ print_bool(PRINT_JSON,
+ "link_pointtopoint", NULL, true);
+ } else
+ print_string(PRINT_FP, NULL, " brd ", NULL);
+
+ print_color_string(PRINT_ANY, COLOR_MAC,
+ "broadcast", "%s",
+ ll_addr_n2a((unsigned char *) &vf_broadcast->broadcast,
+ ifi->ifi_type == ARPHRD_ETHER ?
+ ETH_ALEN : INFINIBAND_ALEN,
+ ifi->ifi_type,
+ b1, sizeof(b1)));
+ }
if (vf[IFLA_VF_VLAN_LIST]) {
struct rtattr *i, *vfvlanlist = vf[IFLA_VF_VLAN_LIST];
@@ -1102,7 +1132,7 @@ int print_linkinfo(struct nlmsghdr *n, void *arg)
open_json_array(PRINT_JSON, "vfinfo_list");
for (i = RTA_DATA(vflist); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
open_json_object(NULL);
- print_vfinfo(fp, i);
+ print_vfinfo(ifi, fp, i);
close_json_object();
}
close_json_array(PRINT_JSON, NULL);
--
2.12.3
^ permalink raw reply related
* [PATCH iproute2-next v4 2/2] uapi: update if_link.h
From: Denis Kirjanov @ 2019-06-28 9:54 UTC (permalink / raw)
To: stephen, dsahern; +Cc: netdev, linux-rdma, dledford, mkubecek, Denis Kirjanov
In-Reply-To: <20190628095426.2819-1-dkirjanov@suse.com>
update if_link.h to commit 75345f888f700c4ab2448287e35d48c760b202e6
("ipoib: show VF broadcast address")
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
include/uapi/linux/if_link.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index bfe7f9e6..5f271d84 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -692,6 +692,7 @@ enum {
IFLA_VF_IB_NODE_GUID, /* VF Infiniband node GUID */
IFLA_VF_IB_PORT_GUID, /* VF Infiniband port GUID */
IFLA_VF_VLAN_LIST, /* nested list of vlans, option for QinQ */
+ IFLA_VF_BROADCAST, /* VF broadcast */
__IFLA_VF_MAX,
};
@@ -702,6 +703,10 @@ struct ifla_vf_mac {
__u8 mac[32]; /* MAX_ADDR_LEN */
};
+struct ifla_vf_broadcast {
+ __u8 broadcast[32];
+};
+
struct ifla_vf_vlan {
__u32 vf;
__u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
--
2.12.3
^ permalink raw reply related
* Re: [PATCH v2 net-next 0/3] Better PHYLINK compliance for SJA1105 DSA
From: Russell King - ARM Linux admin @ 2019-06-28 10:05 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: f.fainelli, vivien.didelot, andrew, davem, netdev
In-Reply-To: <20190627214637.22366-1-olteanv@gmail.com>
On Fri, Jun 28, 2019 at 12:46:34AM +0300, Vladimir Oltean wrote:
> After discussing with Russell King, it appears this driver is making a
> few confusions and not performing some checks for consistent operation.
>
> Changes in v2:
> - Removed redundant print in the phylink_validate callback (in 2/3).
>
> Vladimir Oltean (3):
> net: dsa: sja1105: Don't check state->link in phylink_mac_config
> net: dsa: sja1105: Check for PHY mode mismatches with what PHYLINK
> reports
> net: dsa: sja1105: Mark in-band AN modes not supported for PHYLINK
>
> drivers/net/dsa/sja1105/sja1105_main.c | 56 +++++++++++++++++++++++++-
> 1 file changed, 54 insertions(+), 2 deletions(-)
Thanks. For the whole series:
Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH bpf-next v2] libbpf: add xsk_ring_prod__nb_free() function
From: Magnus Karlsson @ 2019-06-28 10:14 UTC (permalink / raw)
To: Eelco Chaudron
Cc: Network Development, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko
In-Reply-To: <d4692ea57ba7a3fe33549fc6222fb8aea5a4225e.1561537968.git.echaudro@redhat.com>
On Wed, Jun 26, 2019 at 10:33 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>
> When an AF_XDP application received X packets, it does not mean X
> frames can be stuffed into the producer ring. To make it easier for
> AF_XDP applications this API allows them to check how many frames can
> be added into the ring.
>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
>
> v1 -> v2
> - Renamed xsk_ring_prod__free() to xsk_ring_prod__nb_free()
> - Add caching so it will only touch global state when needed
>
> tools/lib/bpf/xsk.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
> index 82ea71a0f3ec..6acb81102346 100644
> --- a/tools/lib/bpf/xsk.h
> +++ b/tools/lib/bpf/xsk.h
> @@ -76,11 +76,11 @@ xsk_ring_cons__rx_desc(const struct xsk_ring_cons *rx, __u32 idx)
> return &descs[idx & rx->mask];
> }
>
> -static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb)
> +static inline __u32 xsk_prod__nb_free(struct xsk_ring_prod *r, __u32 nb)
> {
> __u32 free_entries = r->cached_cons - r->cached_prod;
>
> - if (free_entries >= nb)
> + if (free_entries >= nb && nb != 0)
> return free_entries;
Thanks Eelco for the patch. Is the test nb != 0 introduced here so
that the function will continue with the refresh from the global state
when nb is set to 0? If so, could a user not instead just set the nb
parameter to the size of the ring? This would always trigger a
refresh, except when the number of free entries is equal to the size
of the ring, but then we do not need the refresh anyway. This would
eliminate the nb != 0 test that you introduced from the fast path.
/Magnus
> /* Refresh the local tail pointer.
> @@ -110,7 +110,7 @@ static inline __u32 xsk_cons_nb_avail(struct xsk_ring_cons *r, __u32 nb)
> static inline size_t xsk_ring_prod__reserve(struct xsk_ring_prod *prod,
> size_t nb, __u32 *idx)
> {
> - if (xsk_prod_nb_free(prod, nb) < nb)
> + if (xsk_prod__nb_free(prod, nb) < nb)
> return 0;
>
> *idx = prod->cached_prod;
> --
> 2.20.1
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox