* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once
From: Daniel Borkmann @ 2017-05-10 9:36 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, alexei.starovoitov, john.fastabend, netdev
In-Reply-To: <20170509201842.2ed5e330@cakuba.netronome.com>
On 05/10/2017 05:18 AM, Jakub Kicinski wrote:
> On Wed, 10 May 2017 03:31:31 +0200, Daniel Borkmann wrote:
>> While working on the iproute2 generic XDP frontend, I noticed that
>> as of right now it's possible to have native *and* generic XDP
>> programs loaded both at the same time for the case when a driver
>> supports native XDP.
>
> Nice improvement! A couple of absolute nitpicks below..
>
>> The intended model for generic XDP from b5cdae3291f7 ("net: Generic
>> XDP") is, however, that only one out of the two can be present at
>> once which is also indicated as such in the XPD netlink dump part.
> ^^^
> XDP
Good point.
>> @@ -6851,6 +6851,32 @@ int dev_change_proto_down(struct net_device *dev, bool proto_down)
>> }
>> EXPORT_SYMBOL(dev_change_proto_down);
>>
>> +bool __dev_xdp_attached(struct net_device *dev, xdp_op_t xdp_op)
>
> Out of curiosity - the leading underscores refer to caller having to
> hold rtnl? I assume they are not needed in the function below because
> it's static?
I think I don't quite follow the last question, but it probably makes
sense to add an ASSERT_RTNL() into dev_xdp_attached() inline helper to
make it clearly visible to callers of this api.
>> +{
>> + struct netdev_xdp xdp;
>> +
>> + memset(&xdp, 0, sizeof(xdp));
>> + xdp.command = XDP_QUERY_PROG;
>
> Probably personal preference, but seems like designated struct
> initializer would do quite nicely here and save the memset :)
I had that initially, but I recalled that gcc < 4.6 does not handle this
style for the initialization of anonymous struct/union properly (e.g.,
we fixed that in iproute2 as well). Andrew Morton still uses gcc 4.4.4
and occasionally sends kernel fixes, so we might end up like this anyway.
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index dda9f16..99320f0 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -1251,24 +1251,20 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
>> {
>> struct nlattr *xdp;
>> u32 xdp_flags = 0;
>> - u8 val = 0;
>> int err;
>> + u8 val;
>>
>> xdp = nla_nest_start(skb, IFLA_XDP);
>> if (!xdp)
>> return -EMSGSIZE;
>> +
>> if (rcu_access_pointer(dev->xdp_prog)) {
>> xdp_flags = XDP_FLAGS_SKB_MODE;
>> val = 1;
>> - } else if (dev->netdev_ops->ndo_xdp) {
>> - struct netdev_xdp xdp_op = {};
>> -
>> - xdp_op.command = XDP_QUERY_PROG;
>> - err = dev->netdev_ops->ndo_xdp(dev, &xdp_op);
>> - if (err)
>> - goto err_cancel;
>> - val = xdp_op.prog_attached;
>> + } else {
>> + val = dev_xdp_attached(dev);
>> }
>
> Would it make sense to set xdp_flags to XDP_FLAGS_DRV_MODE here to keep
> things symmetrical? I know you are just preserving existing behaviour
> but it may seem slightly arbitrary to a new comer to report one of the
> very similarly named flags in the dump but not the other.
I thought about it, it's kind of redundant information since
IFLA_XDP_ATTACHED attribute w/o IFLA_XDP_FLAGS attribute today
says that it's native already. It might look strange if we add
also XDP_FLAGS_DRV_MODE there, since it doesn't give anything
new. I rather see it similar to XDP_FLAGS_UPDATE_IF_NOEXIST flag
that is for updating fd only, but I don't really have a strong
opinion on this though. I could add it to the respin if preferred.
^ permalink raw reply
* Re: DQL and TCQ_F_CAN_BYPASS destroy performance under virtualizaiton (Was: "Re: net_sched strange in 4.11")
From: Anton Ivanov @ 2017-05-10 9:42 UTC (permalink / raw)
To: Jason Wang, Stefan Hajnoczi; +Cc: David S. Miller, netdev, Michael S. Tsirkin
In-Reply-To: <6accf7bd-fb21-604b-9586-ecd0f6830a65@redhat.com>
On 10/05/17 09:56, Jason Wang wrote:
>
>
> On 2017年05月10日 13:28, Anton Ivanov wrote:
>> On 10/05/17 03:18, Jason Wang wrote:
>>>
>>> On 2017年05月09日 23:11, Stefan Hajnoczi wrote:
>>>> On Tue, May 09, 2017 at 08:46:46AM +0100, Anton Ivanov wrote:
>>>>> I have figured it out. Two issues.
>>>>>
>>>>> 1) skb->xmit_more is hardly ever set under virtualization because
>>>>> the qdisc
>>>>> is usually bypassed because of TCQ_F_CAN_BYPASS. Once
>>>>> TCQ_F_CAN_BYPASS is
>>>>> set a virtual NIC driver is not likely see skb->xmit_more (this
>>>>> answers my
>>>>> "how does this work at all" question).
>>>>>
>>>>> 2) If that flag is turned off (I patched sched_generic to turn it
>>>>> off in
>>>>> pfifo_fast while testing), DQL keeps xmit_more from being set. If
>>>>> the driver
>>>>> is not DQL enabled xmit_more is never ever set. If the driver is DQL
>>>>> enabled
>>>>> the queue is adjusted to ensure xmit_more stops happening within
>>>>> 10-15 xmit
>>>>> cycles.
>>>>>
>>>>> That is plain *wrong* for virtual NICs - virtio, emulated NICs, etc.
>>>>> There,
>>>>> the BIG cost is telling the hypervisor that it needs to "kick" the
>>>>> packets.
>>>>> The cost of putting them into the vNIC buffers is negligible. You
>>>>> want
>>>>> xmit_more to happen - it makes between 50% and 300% (depending on
>>>>> vNIC
>>>>> design) difference. If there is no xmit_more the vNIC will
>>>>> immediately
>>>>> "kick" the hypervisor and try to signal that the packet needs to
>>>>> move
>>>>> straight away (as for example in virtio_net).
>>> How do you measure the performance? TCP or just measure pps?
>> In this particular case - tcp from guest. I have a couple of other
>> benchmarks (forwarding, etc).
>
> One more question, is the number for virtio-net or other emulated vNIC?
Other for now - you are cc-ed to keep you in the loop.
Virtio is next on my list - I am revisiting the l2tpv3.c driver in QEMU
and looking at how to preserve bulking by adding back sendmmsg (as well
as a list of other features/transports).
We had sendmmsg removed for the final inclusion in QEMU 2.1, it
presently uses only recvmmsg so for the time being it does not care.
That will most likely change once it starts using sendmmsg as well.
>
>>
>>>>> In addition to that, the perceived line rate is proportional to this
>>>>> cost,
>>>>> so I am not sure that the current dql math holds. In fact, I think
>>>>> it does
>>>>> not - it is trying to adjust something which influences the
>>>>> perceived line
>>>>> rate.
>>>>>
>>>>> So - how do we turn BOTH bypass and DQL adjustment while under
>>>>> virtualization and set them to be "always qdisc" + "always xmit_more
>>>>> allowed"
>>> Virtio-net net does not support BQL. Before commit ea7735d97ba9
>>> ("virtio-net: move free_old_xmit_skbs"), it's even impossible to
>>> support that since we don't have tx interrupt for each packet. I
>>> haven't measured the impact of xmit_more, maybe I was wrong but I
>>> think it may help in some cases since it may improve the batching on
>>> host more or less.
>> If you do not support BQL, you might as well look the xmit_more part
>> kick code path. Line 1127.
>>
>> bool kick = !skb->xmit_more; effectively means kick = true;
>>
>> It will never be triggered. You will be kicking each packet and per
>> packet.
>
> Probably not, we have several ways to try to suppress this on the
> virtio layer, host can give hints to disable the kicks through:
>
> - explicitly set a flag
> - implicitly by not publishing a new event idx
>
> FYI, I can get 100-200 packets per vm exit when testing 64 byte
> TCP_STREAM using netperf.
I am aware of that. If, however, the host is providing a hint we might
as well use it.
>
>> xmit_more is now set only out of BQL. If BQL is not enabled you
>> never get it. Now, will the current dql code work correctly if you do
>> not have a defined line rate and completion interrupts - no idea.
>> Probably not. IMHO instead of trying to fix it there should be a way for
>> a device or architecture to turn it off.
>
> In fact BQL is not the only user for xmit_more. Pktgen with burst is
> another. Test does not show obvious difference if I set burst from 0
> to 64 since we already had other ways to avoid kicking host.
That, as well as this not being wired to bulk transport.
>
>>
>> To be clear - I ran into this working on my own drivers for UML, you are
>> cc-ed because you are likely to be one of the most affected.
>
> I'm still not quite sure the issue. Looks like virtio-net is ok since
> BQL is not supported and the impact of xmit_more could be ignored.
Presently - yes. If you have bulk aware transports to wire into that is
likely to make a difference.
>
> Thanks
>
>>
>> A.
>>
>>> Thanks
>>>
>>>>> A.
>>>>>
>>>>> P.S. Cc-ing virtio maintainer
>>>> CCing Michael Tsirkin and Jason Wang, who are the core virtio and
>>>> virtio-net maintainers. (I maintain the vsock driver - it's unrelated
>>>> to this discussion.)
>>>>
>>>>> A.
>>>>>
>>>>>
>>>>> On 08/05/17 08:15, Anton Ivanov wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> I was revising some of my old work for UML to prepare it for
>>>>>> submission
>>>>>> and I noticed that skb->xmit_more does not seem to be set any more.
>>>>>>
>>>>>> I traced the issue as far as net/sched/sched_generic.c
>>>>>>
>>>>>> try_bulk_dequeue_skb() is never invoked (the drivers I am working
>>>>>> on are
>>>>>> dql enabled so that is not the problem).
>>>>>>
>>>>>> More interestingly, if I put a breakpoint and debug output into
>>>>>> dequeue_skb() around line 147 - right before the bulk: tag that skb
>>>>>> there is always NULL. ???
>>>>>>
>>>>>> Similarly, debug in pfifo_fast_dequeue shows only NULLs being
>>>>>> dequeued.
>>>>>> Again - ???
>>>>>>
>>>>>> First and foremost, I apologize for the silly question, but how can
>>>>>> this
>>>>>> work at all? I see the skbs showing up at the driver level, why are
>>>>>> NULLs being returned at qdisc dequeue and where do the skbs at the
>>>>>> driver level come from?
>>>>>>
>>>>>> Second, where should I look to fix it?
>>>>>>
>>>>>> A.
>>>>>>
>>>>> --
>>>>> Anton R. Ivanov
>>>>>
>>>>> Cambridge Greys Limited, England company No 10273661
>>>>> http://www.cambridgegreys.com/
>>>>>
>>>
>>
>
>
--
Anton R. Ivanov
Cambridge Greys Limited, England company No 10273661
http://www.cambridgegreys.com/
^ permalink raw reply
* Re: [PATCH] ip: mpls: fix printing of mpls labels
From: Simon Horman @ 2017-05-10 10:02 UTC (permalink / raw)
To: David Ahern; +Cc: stephen, netdev, roopa
In-Reply-To: <20170509060413.11596-1-dsahern@gmail.com>
On Mon, May 08, 2017 at 11:04:13PM -0700, David Ahern wrote:
> If the kernel returns more labels than iproute2 expects, none of
> the labels are printed and (null) is shown instead:
> $ ip -f mpls ro ls
> 101 as to (null) via inet 172.16.2.2 dev virt12
> 201 as to 202/203 via inet6 2001:db8:2::2 dev virt12
>
> Remove the use of MPLS_MAX_LABELS and rely on buffer length that is
> passed to mpls_ntop. With this change ip can print the label stack
> returned by the kernel up to 255 characters (limit is due to size of
> buf passed in) which amounts to 31 labels with a separator.
>
> With this change the above is:
> $ ip/ip -f mpls ro ls
> 101 as to 102/103/104/105/106/107/108/109/110 via inet 172.16.2.2 dev virt12
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
^ permalink raw reply
* Re: iproute2 ss outputs duplicate tcp sockets info on kernel 3.10.105
From: Phil Sutter @ 2017-05-10 10:37 UTC (permalink / raw)
To: Li Er; +Cc: netdev, Cyrill Gorcunov
In-Reply-To: <591121b4.213b9f0a.5aed.b3a8@mx.google.com>
Hi,
Cc'ing Cyrill who wrote the code in question. Maybe he has an idea
what's going wrong here.
Cheers, Phil
On Mon, May 08, 2017 at 06:56:04PM -0700, Li Er wrote:
> i'm using v4.11.0 release of iproute2 and kernel 3.10.105, simply
> running
>
> $ ss
> Netid State Recv-Q Send-Q Local Address:Port Peer Address:Port
> tcp CLOSE-WAIT 434 0 10.0.0.1:47931 65.49.18.136:https
> tcp CLOSE-WAIT 432 0 10.0.0.1:47932 65.49.18.136:https
> tcp CLOSE-WAIT 434 0 10.0.0.1:47931 65.49.18.136:https
> tcp CLOSE-WAIT 432 0 10.0.0.1:47932 65.49.18.136:https
>
> as you can see, there's one duplicate entry of each tcp socket,
> however, if i explicitly specify tcp socket by adding the -t
> switch,
>
> $ ss -t
> State Recv-Q Send-Q Local Address:Port Peer Address:Port
> CLOSE-WAIT 434 0 10.0.0.1:47931 65.49.18.136:https
> CLOSE-WAIT 432 0 10.0.0.1:47932 65.49.18.136:https
>
> the duplication is gone.
>
> this problem also occurs on git master, but not on iproute2
> v4.3.0, so i did a git bisect and found out the commit which
> caused this is 9f66764e308e9c645b3fb2d1cfbb7fb207eb5de1, and by
> revert this commit on git master, i.e. removing
>
> err = rtnl_dump_done(rth, h);
> if (err < 0)
> return -1;
>
> these 3 lines of code of lib/libnetlink.c, the problem is gone.
>
> since i'm not familiar with the source code, i doubt this is the
> right way to solve the problem, what's your suggestions? thanks.
^ permalink raw reply
* RE: bpf pointer alignment validation
From: David Laight @ 2017-05-10 11:12 UTC (permalink / raw)
To: 'Alexei Starovoitov', David Miller
Cc: daniel@iogearbox.net, ast@fb.com, netdev@vger.kernel.org
In-Reply-To: <20170510055735.hfkoh4w3xaka5yl5@ast-mbp>
From: Alexei Starovoitov
> Sent: 10 May 2017 06:58
> > +static u32 calc_align(u32 imm)
> > +{
> > + u32 align = 1;
> > +
> > + if (!imm)
> > + return 1U << 31;
> > +
> > + while (!(imm & 1)) {
> > + imm >>= 1;
> > + align <<= 1;
> > + }
> > + return align;
> > +}
>
> same question as in previous reply.
> Why not to use something like:
> static u32 calc_align(u32 n)
> {
> if (!n)
> return 1U << 31;
> return n - ((n - 1) & n);
> }
That function needs a comment saying what it returns.
Think I'd write it as:
return n & ~(n & (n - 1));
(even though that might be one more instruction)
David
^ permalink raw reply
* Re: [PATCH 1/2] net: Added mtu parameter to dev_forward_skb calls
From: kbuild test robot @ 2017-05-10 11:47 UTC (permalink / raw)
To: Fredrik Markstrom
Cc: kbuild-all, Eric Dumazet, David S. Miller, Stephen Hemminger,
Alexei Starovoitov, Daniel Borkmann, netdev, linux-kernel, bridge,
Fredrik Markström
In-Reply-To: <20170509124439.45674-2-fredrik.markstrom@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]
Hi Fredrik,
[auto build test ERROR on net/master]
[also build test ERROR on v4.11 next-20170510]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Fredrik-Markstrom/net-Added-mtu-parameter-to-dev_forward_skb-calls/20170509-231142
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
net//bridge/br_forward.c: In function '__br_forward':
>> net//bridge/br_forward.c:99:46: error: 'struct sk_buff' has no member named 'dev_mtu'
if (!is_skb_forwardable(skb->dev, skb, skb->dev_mtu)) {
^~
vim +99 net//bridge/br_forward.c
93 }
94 br_hook = NF_BR_FORWARD;
95 skb_forward_csum(skb);
96 net = dev_net(indev);
97 } else {
98 if (unlikely(netpoll_tx_running(to->br->dev))) {
> 99 if (!is_skb_forwardable(skb->dev, skb, skb->dev_mtu)) {
100 kfree_skb(skb);
101 } else {
102 skb_push(skb, ETH_HLEN);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39200 bytes --]
^ permalink raw reply
* Your response Is highly appreciated!
From: Mr.Adams Salem @ 2017-05-10 12:08 UTC (permalink / raw)
Hello ,
I am specifically contacting you in respect of a business proposal that I have for you as you appear very relevant in the proposal.
Please kindly reply back to me for further details.
Waiting to hear from you.
Regards,
Mr.Adams Salem
Email: mradams@salem-my.com
^ permalink raw reply
* Re: [PATCH 1/3] ptr_ring: batch ring zeroing
From: Michael S. Tsirkin @ 2017-05-10 12:20 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: linux-kernel, Jason Wang, netdev@vger.kernel.org, John Fastabend
In-Reply-To: <20170510111813.35f21ab0@redhat.com>
On Wed, May 10, 2017 at 11:18:13AM +0200, Jesper Dangaard Brouer wrote:
> On Tue, 9 May 2017 16:33:14 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Sat, Apr 08, 2017 at 02:14:08PM +0200, Jesper Dangaard Brouer wrote:
> > > On Fri, 7 Apr 2017 08:49:57 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > A known weakness in ptr_ring design is that it does not handle well the
> > > > situation when ring is almost full: as entries are consumed they are
> > > > immediately used again by the producer, so consumer and producer are
> > > > writing to a shared cache line.
> > > >
> > > > To fix this, add batching to consume calls: as entries are
> > > > consumed do not write NULL into the ring until we get
> > > > a multiple (in current implementation 2x) of cache lines
> > > > away from the producer. At that point, write them all out.
> > > >
> > > > We do the write out in the reverse order to keep
> > > > producer from sharing cache with consumer for as long
> > > > as possible.
> > > >
> > > > Writeout also triggers when ring wraps around - there's
> > > > no special reason to do this but it helps keep the code
> > > > a bit simpler.
> > > >
> > > > What should we do if getting away from producer by 2 cache lines
> > > > would mean we are keeping the ring moe than half empty?
> > > > Maybe we should reduce the batching in this case,
> > > > current patch simply reduces the batching.
> > > >
> > > > Notes:
> > > > - it is no longer true that a call to consume guarantees
> > > > that the following call to produce will succeed.
> > > > No users seem to assume that.
> > > > - batching can also in theory reduce the signalling rate:
> > > > users that would previously send interrups to the producer
> > > > to wake it up after consuming each entry would now only
> > > > need to do this once in a batch.
> > > > Doing this would be easy by returning a flag to the caller.
> > > > No users seem to do signalling on consume yet so this was not
> > > > implemented yet.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >
> > > > Jason, I am curious whether the following gives you some of
> > > > the performance boost that you see with vhost batching
> > > > patches. Is vhost batching on top still helpful?
> > > >
> > > > include/linux/ptr_ring.h | 63 +++++++++++++++++++++++++++++++++++++++++-------
> > > > 1 file changed, 54 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > > > index 6c70444..6b2e0dd 100644
> > > > --- a/include/linux/ptr_ring.h
> > > > +++ b/include/linux/ptr_ring.h
> > > > @@ -34,11 +34,13 @@
> > > > struct ptr_ring {
> > > > int producer ____cacheline_aligned_in_smp;
> > > > spinlock_t producer_lock;
> > > > - int consumer ____cacheline_aligned_in_smp;
> > > > + int consumer_head ____cacheline_aligned_in_smp; /* next valid entry */
> > > > + int consumer_tail; /* next entry to invalidate */
> > > > spinlock_t consumer_lock;
> > > > /* Shared consumer/producer data */
> > > > /* Read-only by both the producer and the consumer */
> > > > int size ____cacheline_aligned_in_smp; /* max entries in queue */
> > > > + int batch; /* number of entries to consume in a batch */
> > > > void **queue;
> > > > };
> > > >
> > > > @@ -170,7 +172,7 @@ static inline int ptr_ring_produce_bh(struct ptr_ring *r, void *ptr)
> > > > static inline void *__ptr_ring_peek(struct ptr_ring *r)
> > > > {
> > > > if (likely(r->size))
> > > > - return r->queue[r->consumer];
> > > > + return r->queue[r->consumer_head];
> > > > return NULL;
> > > > }
> > > >
> > > > @@ -231,9 +233,38 @@ static inline bool ptr_ring_empty_bh(struct ptr_ring *r)
> > > > /* Must only be called after __ptr_ring_peek returned !NULL */
> > > > static inline void __ptr_ring_discard_one(struct ptr_ring *r)
> > > > {
> > > > - r->queue[r->consumer++] = NULL;
> > > > - if (unlikely(r->consumer >= r->size))
> > > > - r->consumer = 0;
> > > > + /* Fundamentally, what we want to do is update consumer
> > > > + * index and zero out the entry so producer can reuse it.
> > > > + * Doing it naively at each consume would be as simple as:
> > > > + * r->queue[r->consumer++] = NULL;
> > > > + * if (unlikely(r->consumer >= r->size))
> > > > + * r->consumer = 0;
> > > > + * but that is suboptimal when the ring is full as producer is writing
> > > > + * out new entries in the same cache line. Defer these updates until a
> > > > + * batch of entries has been consumed.
> > > > + */
> > > > + int head = r->consumer_head++;
> > > > +
> > > > + /* Once we have processed enough entries invalidate them in
> > > > + * the ring all at once so producer can reuse their space in the ring.
> > > > + * We also do this when we reach end of the ring - not mandatory
> > > > + * but helps keep the implementation simple.
> > > > + */
> > > > + if (unlikely(r->consumer_head - r->consumer_tail >= r->batch ||
> > > > + r->consumer_head >= r->size)) {
> > > > + /* Zero out entries in the reverse order: this way we touch the
> > > > + * cache line that producer might currently be reading the last;
> > > > + * producer won't make progress and touch other cache lines
> > > > + * besides the first one until we write out all entries.
> > > > + */
> > > > + while (likely(head >= r->consumer_tail))
> > > > + r->queue[head--] = NULL;
> > > > + r->consumer_tail = r->consumer_head;
> > > > + }
> > > > + if (unlikely(r->consumer_head >= r->size)) {
> > > > + r->consumer_head = 0;
> > > > + r->consumer_tail = 0;
> > > > + }
> > > > }
> > >
> > > I love this idea. Reviewed and discussed the idea in-person with MST
> > > during netdevconf[1] at this laptop. I promised I will also run it
> > > through my micro-benchmarking[2] once I return home (hint ptr_ring gets
> > > used in network stack as skb_array).
> >
> > I'm merging this through my tree. Any objections?
>
> I just did the micro-benchmark evaluation I promised and everything
> looks good, so no objections from me.
>
> John Fastabend recently posted a RFC patchset for removing the qdisc
> lock. The main purpose is to separate enqueue'ers and dequeue'ers from
> serializing on the same lock. But we need a new queue implementation
> that avoids the false-sharing between enq+deq.
>
> This is why John choose to use ptr_ring, changed (pfifo_fast) qdisc to
> use this ptr_ring. I think this patch might help his overload testing,
> as my theory is that he is hitting false-sharing on the queue, due to
> the queue always being full.
>
>
> > > Reviewed-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > >
> > > [1] http://netdevconf.org/2.1/
> > > [2] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/skb_array_bench01.c
>
> If you like can also add my:
>
> Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>
I pushed it out already unfortunately so I can't attach that.
Sorry. Thanks a lot for the testing!
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
>
> $ modprobe skb_array_test01
> $ dmesg
> [73228.381497] skb_array_test01: Loaded
> [73228.381498] skb_array_test01: PASSED - basic_init_and_cleanup()
> [73228.381498] skb_array_test01: PASSED - basic_add_and_remove_object()
> [73228.381505] skb_array_test01: PASSED - test_queue_full_condition()
> [73228.381505] skb_array_test01: PASSED - test_queue_empty_condition()
> [73228.381510] skb_array_test01: PASSED - test_queue_resize()
^ permalink raw reply
* [PULL] virtio: fixes, cleanups, performance
From: Michael S. Tsirkin @ 2017-05-10 12:24 UTC (permalink / raw)
To: Linus Torvalds
Cc: kvm, virtualization, netdev, linux-kernel, borntraeger,
colin.king, cornelia.huck, dan.carpenter, jasowang, mst, nsekhar,
pasic
The following changes since commit a351e9b9fc24e982ec2f0e76379a49826036da12:
Linux 4.11 (2017-04-30 19:47:48 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to c8b0d7290657996a29f318b948d2397d30e70c36:
s390/virtio: change maintainership (2017-05-09 16:43:25 +0300)
----------------------------------------------------------------
virtio: fixes, cleanups, performance
A bunch of changes to virtio, most affecting virtio net.
ptr_ring batched zeroing - first of batching enhancements
that seems ready.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Christian Borntraeger (1):
s390/virtio: change maintainership
Colin Ian King (1):
tools/virtio: fix spelling mistake: "wakeus" -> "wakeups"
Cornelia Huck (1):
virtio: virtio_driver doc
Dan Carpenter (2):
ringtest: fix an assert statement
virtio_net: tidy a couple debug statements
Michael S. Tsirkin (11):
virtio: wrap find_vqs
virtio: add context flag to find vqs
virtio: allow extra context per descriptor
virtio_net: allow specifying context for rx
virtio_net: rework mergeable buffer handling
virtio_net: reduce alignment for buffers
virtio_net: fix support for small rings
virtio_net: don't reset twice on XDP on/off
ptr_ring: batch ring zeroing
ringtest: support test specific parameters
ptr_ring: support testing different batching sizes
Sekhar Nori (1):
tools/virtio: fix build breakage
MAINTAINERS | 2 +-
drivers/block/virtio_blk.c | 3 +-
drivers/char/virtio_console.c | 6 +-
drivers/crypto/virtio/virtio_crypto_core.c | 3 +-
drivers/gpu/drm/virtio/virtgpu_kms.c | 3 +-
drivers/misc/mic/vop/vop_main.c | 9 +-
drivers/net/caif/caif_virtio.c | 3 +-
drivers/net/virtio_net.c | 147 ++++++++++++++++-------------
drivers/remoteproc/remoteproc_virtio.c | 10 +-
drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
drivers/s390/virtio/kvm_virtio.c | 8 +-
drivers/s390/virtio/virtio_ccw.c | 7 +-
drivers/scsi/virtio_scsi.c | 3 +-
drivers/virtio/virtio_balloon.c | 3 +-
drivers/virtio/virtio_input.c | 3 +-
drivers/virtio/virtio_mmio.c | 8 +-
drivers/virtio/virtio_pci_common.c | 17 ++--
drivers/virtio/virtio_pci_common.h | 4 +-
drivers/virtio/virtio_pci_legacy.c | 4 +-
drivers/virtio/virtio_pci_modern.c | 12 ++-
drivers/virtio/virtio_ring.c | 77 ++++++++++++---
include/linux/ptr_ring.h | 63 +++++++++++--
include/linux/virtio.h | 13 +++
include/linux/virtio_config.h | 25 ++++-
include/linux/virtio_ring.h | 3 +
net/vmw_vsock/virtio_transport.c | 6 +-
tools/virtio/linux/virtio.h | 1 +
tools/virtio/ringtest/main.c | 15 ++-
tools/virtio/ringtest/main.h | 2 +
tools/virtio/ringtest/ptr_ring.c | 3 +
tools/virtio/virtio_test.c | 4 +-
tools/virtio/vringh_test.c | 7 +-
32 files changed, 330 insertions(+), 146 deletions(-)
^ permalink raw reply
* Re: [PATCH net-next V4 10/10] vhost_net: try batch dequing from skb array
From: Michael S. Tsirkin @ 2017-05-10 12:34 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel
In-Reply-To: <1494387382-19916-11-git-send-email-jasowang@redhat.com>
On Wed, May 10, 2017 at 11:36:22AM +0800, Jason Wang wrote:
> We used to dequeue one skb during recvmsg() from skb_array, this could
> be inefficient because of the bad cache utilization and spinlock
> touching for each packet. This patch tries to batch them by calling
> batch dequeuing helpers explicitly on the exported skb array and pass
> the skb back through msg_control for underlayer socket to finish the
> userspace copying.
>
> Batch dequeuing is also the requirement for more batching improvement
> on rx.
>
> Tests were done by pktgen on tap with XDP1 in guest on top of batch
> zeroing:
>
> rx batch | pps
>
> 256 2.41Mpps (+6.16%)
> 128 2.48Mpps (+8.80%)
> 64 2.38Mpps (+3.96%) <- Default
> 16 2.31Mpps (+1.76%)
> 4 2.31Mpps (+1.76%)
> 1 2.30Mpps (+1.32%)
> 0 2.27Mpps (+7.48%)
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vhost/net.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 111 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 9b51989..fbaecf3 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -28,6 +28,8 @@
> #include <linux/if_macvlan.h>
> #include <linux/if_tap.h>
> #include <linux/if_vlan.h>
> +#include <linux/skb_array.h>
> +#include <linux/skbuff.h>
>
> #include <net/sock.h>
>
> @@ -85,6 +87,13 @@ struct vhost_net_ubuf_ref {
> struct vhost_virtqueue *vq;
> };
>
> +#define VHOST_RX_BATCH 64
> +struct vhost_net_buf {
> + struct sk_buff *queue[VHOST_RX_BATCH];
> + int tail;
> + int head;
> +};
> +
Do you strictly need to put this inline? This structure is quite big
already. Do you see a measureabe difference if you make it
struct sk_buff **queue;
int tail;
int head;
?
Will also make it easier to play with the size in the future
should someone want to see how does it work e.g. for different
ring sizes.
> struct vhost_net_virtqueue {
> struct vhost_virtqueue vq;
> size_t vhost_hlen;
> @@ -99,6 +108,8 @@ struct vhost_net_virtqueue {
> /* Reference counting for outstanding ubufs.
> * Protected by vq mutex. Writers must also take device mutex. */
> struct vhost_net_ubuf_ref *ubufs;
> + struct skb_array *rx_array;
> + struct vhost_net_buf rxq;
> };
>
> struct vhost_net {
> @@ -117,6 +128,71 @@ struct vhost_net {
>
> static unsigned vhost_net_zcopy_mask __read_mostly;
>
> +static void *vhost_net_buf_get_ptr(struct vhost_net_buf *rxq)
> +{
> + if (rxq->tail != rxq->head)
> + return rxq->queue[rxq->head];
> + else
> + return NULL;
> +}
> +
> +static int vhost_net_buf_get_size(struct vhost_net_buf *rxq)
> +{
> + return rxq->tail - rxq->head;
> +}
> +
> +static int vhost_net_buf_is_empty(struct vhost_net_buf *rxq)
> +{
> + return rxq->tail == rxq->head;
> +}
> +
> +static void *vhost_net_buf_consume(struct vhost_net_buf *rxq)
> +{
> + void *ret = vhost_net_buf_get_ptr(rxq);
> + ++rxq->head;
> + return ret;
> +}
> +
> +static int vhost_net_buf_produce(struct vhost_net_virtqueue *nvq)
> +{
> + struct vhost_net_buf *rxq = &nvq->rxq;
> +
> + rxq->head = 0;
> + rxq->tail = skb_array_consume_batched(nvq->rx_array, rxq->queue,
> + VHOST_RX_BATCH);
> + return rxq->tail;
> +}
> +
> +static void vhost_net_buf_unproduce(struct vhost_net_virtqueue *nvq)
> +{
> + struct vhost_net_buf *rxq = &nvq->rxq;
> +
> + if (nvq->rx_array && !vhost_net_buf_is_empty(rxq)) {
> + skb_array_unconsume(nvq->rx_array, rxq->queue + rxq->head,
> + vhost_net_buf_get_size(rxq));
> + rxq->head = rxq->tail = 0;
> + }
> +}
> +
> +static int vhost_net_buf_peek(struct vhost_net_virtqueue *nvq)
> +{
> + struct vhost_net_buf *rxq = &nvq->rxq;
> +
> + if (!vhost_net_buf_is_empty(rxq))
> + goto out;
> +
> + if (!vhost_net_buf_produce(nvq))
> + return 0;
> +
> +out:
> + return __skb_array_len_with_tag(vhost_net_buf_get_ptr(rxq));
> +}
> +
> +static void vhost_net_buf_init(struct vhost_net_buf *rxq)
> +{
> + rxq->head = rxq->tail = 0;
> +}
> +
> static void vhost_net_enable_zcopy(int vq)
> {
> vhost_net_zcopy_mask |= 0x1 << vq;
> @@ -201,6 +277,7 @@ static void vhost_net_vq_reset(struct vhost_net *n)
> n->vqs[i].ubufs = NULL;
> n->vqs[i].vhost_hlen = 0;
> n->vqs[i].sock_hlen = 0;
> + vhost_net_buf_init(&n->vqs[i].rxq);
> }
>
> }
> @@ -503,15 +580,14 @@ static void handle_tx(struct vhost_net *net)
> mutex_unlock(&vq->mutex);
> }
>
> -static int peek_head_len(struct sock *sk)
> +static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
> {
> - struct socket *sock = sk->sk_socket;
> struct sk_buff *head;
> int len = 0;
> unsigned long flags;
>
> - if (sock->ops->peek_len)
> - return sock->ops->peek_len(sock);
> + if (rvq->rx_array)
> + return vhost_net_buf_peek(rvq);
>
> spin_lock_irqsave(&sk->sk_receive_queue.lock, flags);
> head = skb_peek(&sk->sk_receive_queue);
> @@ -537,10 +613,11 @@ static int sk_has_rx_data(struct sock *sk)
>
> static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
> {
> + struct vhost_net_virtqueue *rvq = &net->vqs[VHOST_NET_VQ_RX];
> struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
> struct vhost_virtqueue *vq = &nvq->vq;
> unsigned long uninitialized_var(endtime);
> - int len = peek_head_len(sk);
> + int len = peek_head_len(rvq, sk);
>
> if (!len && vq->busyloop_timeout) {
> /* Both tx vq and rx socket were polled here */
> @@ -561,7 +638,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
> vhost_poll_queue(&vq->poll);
> mutex_unlock(&vq->mutex);
>
> - len = peek_head_len(sk);
> + len = peek_head_len(rvq, sk);
> }
>
> return len;
> @@ -699,6 +776,8 @@ static void handle_rx(struct vhost_net *net)
> /* On error, stop handling until the next kick. */
> if (unlikely(headcount < 0))
> goto out;
> + if (nvq->rx_array)
> + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
> /* On overrun, truncate and discard */
> if (unlikely(headcount > UIO_MAXIOV)) {
> iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> @@ -841,6 +920,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
> n->vqs[i].done_idx = 0;
> n->vqs[i].vhost_hlen = 0;
> n->vqs[i].sock_hlen = 0;
> + vhost_net_buf_init(&n->vqs[i].rxq);
> }
> vhost_dev_init(dev, vqs, VHOST_NET_VQ_MAX);
>
> @@ -856,11 +936,14 @@ static struct socket *vhost_net_stop_vq(struct vhost_net *n,
> struct vhost_virtqueue *vq)
> {
> struct socket *sock;
> + struct vhost_net_virtqueue *nvq =
> + container_of(vq, struct vhost_net_virtqueue, vq);
>
> mutex_lock(&vq->mutex);
> sock = vq->private_data;
> vhost_net_disable_vq(n, vq);
> vq->private_data = NULL;
> + vhost_net_buf_unproduce(nvq);
> mutex_unlock(&vq->mutex);
> return sock;
> }
> @@ -953,6 +1036,25 @@ static struct socket *get_raw_socket(int fd)
> return ERR_PTR(r);
> }
>
> +static struct skb_array *get_tap_skb_array(int fd)
> +{
> + struct skb_array *array;
> + struct file *file = fget(fd);
> +
> + if (!file)
> + return NULL;
> + array = tun_get_skb_array(file);
> + if (!IS_ERR(array))
> + goto out;
> + array = tap_get_skb_array(file);
> + if (!IS_ERR(array))
> + goto out;
> + array = NULL;
> +out:
> + fput(file);
> + return array;
> +}
> +
> static struct socket *get_tap_socket(int fd)
> {
> struct file *file = fget(fd);
> @@ -1029,6 +1131,9 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
>
> vhost_net_disable_vq(n, vq);
> vq->private_data = sock;
> + vhost_net_buf_unproduce(nvq);
> + if (index == VHOST_NET_VQ_RX)
> + nvq->rx_array = get_tap_skb_array(fd);
> r = vhost_vq_init_access(vq);
> if (r)
> goto err_used;
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next V4 00/10] vhost_net batch dequeuing
From: Michael S. Tsirkin @ 2017-05-10 12:37 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel
In-Reply-To: <1494387382-19916-1-git-send-email-jasowang@redhat.com>
On Wed, May 10, 2017 at 11:36:12AM +0800, Jason Wang wrote:
> This series tries to implement rx batching for vhost-net. This is done
> by batching the dequeuing from skb_array which was exported by
> underlayer socket and pass the sbk back through msg_control to finish
> userspace copying. This is also the requirement for more batching
> implemention on rx path.
>
> Tests shows at most 8.8% improvment bon rx pps on top of batch zeroing.
>
> Please review.
>
> Thanks
>
> Changes from V3:
> - add batch zeroing patch to fix the build warnings
FYI that one's going upstream now.
> Changes from V2:
> - rebase to net-next HEAD
> - use unconsume helpers to put skb back on releasing
> - introduce and use vhost_net internal buffer helpers
> - renew performance numbers on top of batch zeroing
>
> Changes from V1:
> - switch to use for() in __ptr_ring_consume_batched()
> - rename peek_head_len_batched() to fetch_skbs()
> - use skb_array_consume_batched() instead of
> skb_array_consume_batched_bh() since no consumer run in bh
> - drop the lockless peeking patch since skb_array could be resized, so
> it's not safe to call lockless one
>
> Jason Wang (8):
> skb_array: introduce skb_array_unconsume
> ptr_ring: introduce batch dequeuing
> skb_array: introduce batch dequeuing
> tun: export skb_array
> tap: export skb_array
> tun: support receiving skb through msg_control
> tap: support receiving skb from msg_control
> vhost_net: try batch dequing from skb array
>
> Michael S. Tsirkin (2):
> ptr_ring: batch ring zeroing
> ptr_ring: add ptr_ring_unconsume
>
> drivers/net/tap.c | 25 ++++++-
> drivers/net/tun.c | 31 ++++++--
> drivers/vhost/net.c | 117 +++++++++++++++++++++++++++--
> include/linux/if_tap.h | 5 ++
> include/linux/if_tun.h | 5 ++
> include/linux/ptr_ring.h | 183 +++++++++++++++++++++++++++++++++++++++++++---
> include/linux/skb_array.h | 31 ++++++++
> 7 files changed, 370 insertions(+), 27 deletions(-)
>
> --
> 2.7.4
^ permalink raw reply
* [PATCH] mdio: mux: Correct mdio_mux_init error path issues
From: Jon Mason @ 2017-05-10 15:20 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, linux-kernel, bcm-kernel-feedback-list
There is a potential unnecessary refcount decriment on error path of
put_device(&pb->mii_bus->dev), as it is possible to avoid the
of_mdio_find_bus() call if mux_bus is specified by the calling function.
The same put_device() is not called in the error path if the
devm_kzalloc of pb fails. This caused the variable used in the
put_device() to be changed, as the pb pointer was obviously not set up.
There is an unnecessary of_node_get() on child_bus_node if the
of_mdiobus_register() is successful, as the
for_each_available_child_of_node() automatically increments this.
Thus the refcount on this node will always be +1 more than it should be.
There is no of_node_put() on child_bus_node if the of_mdiobus_register()
call fails.
Finally, it is lacking devm_kfree() of pb in the error path. While this
might not be technically necessary, it was present in other parts of the
function. So, I am adding it where necessary to make it uniform.
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
Fixes: f20e6657a875 ("mdio: mux: Enhanced MDIO mux framework for integrated multiplexers")
Fixes: 0ca2997d1452 ("netdev/of/phy: Add MDIO bus multiplexer support.")
---
drivers/net/phy/mdio-mux.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/mdio-mux.c b/drivers/net/phy/mdio-mux.c
index 963838d4fac1..6943c5ece44a 100644
--- a/drivers/net/phy/mdio-mux.c
+++ b/drivers/net/phy/mdio-mux.c
@@ -122,10 +122,9 @@ int mdio_mux_init(struct device *dev,
pb = devm_kzalloc(dev, sizeof(*pb), GFP_KERNEL);
if (pb == NULL) {
ret_val = -ENOMEM;
- goto err_parent_bus;
+ goto err_pb_kz;
}
-
pb->switch_data = data;
pb->switch_fn = switch_fn;
pb->current_child = -1;
@@ -154,6 +153,7 @@ int mdio_mux_init(struct device *dev,
cb->mii_bus = mdiobus_alloc();
if (!cb->mii_bus) {
ret_val = -ENOMEM;
+ devm_kfree(dev, cb);
of_node_put(child_bus_node);
break;
}
@@ -169,8 +169,8 @@ int mdio_mux_init(struct device *dev,
if (r) {
mdiobus_free(cb->mii_bus);
devm_kfree(dev, cb);
+ of_node_put(child_bus_node);
} else {
- of_node_get(child_bus_node);
cb->next = pb->children;
pb->children = cb;
}
@@ -181,9 +181,11 @@ int mdio_mux_init(struct device *dev,
return 0;
}
+ devm_kfree(dev, pb);
+err_pb_kz:
/* balance the reference of_mdio_find_bus() took */
- put_device(&pb->mii_bus->dev);
-
+ if (!mux_bus)
+ put_device(&parent_bus->dev);
err_parent_bus:
of_node_put(parent_bus_node);
return ret_val;
--
2.7.4
^ permalink raw reply related
* Re: bpf pointer alignment validation
From: David Miller @ 2017-05-10 15:33 UTC (permalink / raw)
To: alexei.starovoitov; +Cc: daniel, ast, netdev
In-Reply-To: <20170510055735.hfkoh4w3xaka5yl5@ast-mbp>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Tue, 9 May 2017 22:57:37 -0700
> On Tue, May 09, 2017 at 02:32:34PM -0400, David Miller wrote:
>>
>> +static u32 calc_align(u32 imm)
>> +{
>> + u32 align = 1;
>> +
>> + if (!imm)
>> + return 1U << 31;
>> +
>> + while (!(imm & 1)) {
>> + imm >>= 1;
>> + align <<= 1;
>> + }
>> + return align;
>> +}
>
> same question as in previous reply.
> Why not to use something like:
> static u32 calc_align(u32 n)
> {
> if (!n)
> return 1U << 31;
> return n - ((n - 1) & n);
> }
Ok.
I did a cursory search and we don't have a generic kernel helper for
this kind of calculation. I was actually quite surprised, as we
have one for everything else :-)
> this needs to be tweaked like
> if (log_level > 1)
> verbose("%d:", insn_idx);
> else
> verbose("\nfrom %d to %d:", prev_insn_idx, insn_idx);
>
> otherwise it prints prev_insn_idx which is meaningful
> only with processing exit and search pruning.
Agreed.
> Nice to see all these comments.
> I wonder how we can make them automatic in the verifier.
> Like if verifier can somehow hint the user in such human friendly way
> about what is happening with the program.
> Today that's the #1 problem. Most folks complaining
> that verifier error messages are too hard to understand.
We can put whatever text strings we want into that verifier buffer.
It is as flexible as netlink extended acks.
> would it make sense to bpf_prog_test_run() it here as well?
We could.
> On x86 not much value, but on sparc we can somehow look for traps?
> Is there some counter of unaligned traps that we can read and report
> as error to user space after prog_test_run ?
Unfortunately, no.
> These tests we cannot really run, since they don't do any load/store.
> I mean more for some future tests. Or some sort of debug warn
> that there were traps while bpf prog was executed, so the user
> is alarmed and reports to us, since that would be a bug in verifier
> align logic?
One thing I could definitely do is add logic to the unaligned trap
handler to print something special in the logs if we find that we
are executing BPF code.
The basic structure of the log message can be codified in a generic
bpf_xxx() helper, which architectures call with the PC and unaligned
address as arguments.
I was thinking more last night about strict alignment mode for the
verifier, so that bugs can be spotted on all architectures. But
it stupidly will not work.
The problem is that x86 and powerpc define NET_IP_ALIGN as 0, so all
bets are off.
One thing we could do in "strict alignment" verifier mode is pretend
that NET_IP_ALIGN is 2 in the alignment checks.
^ permalink raw reply
* [net-mellanox] question about potential null pointer dereference
From: Gustavo A. R. Silva @ 2017-05-10 15:36 UTC (permalink / raw)
To: Jiri Pirko, Ido Schimmel; +Cc: netdev, linux-kernel
Hello everybody,
While looking into Coverity ID 1350941 I ran into the following piece
of code at
drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c:1483:
1483static void mlxsw_sp_fdb_notify_mac_process(struct mlxsw_sp *mlxsw_sp,
1484 char *sfn_pl, int rec_index,
1485 bool adding)
1486{
1487 struct mlxsw_sp_port *mlxsw_sp_port;
1488 char mac[ETH_ALEN];
1489 u8 local_port;
1490 u16 vid, fid;
1491 bool do_notification = true;
1492 int err;
1493
1494 mlxsw_reg_sfn_mac_unpack(sfn_pl, rec_index, mac, &fid,
&local_port);
1495 mlxsw_sp_port = mlxsw_sp->ports[local_port];
1496 if (!mlxsw_sp_port) {
1497 dev_err_ratelimited(mlxsw_sp->bus_info->dev,
"Incorrect local port in FDB notification\n");
1498 goto just_remove;
1499 }
1500
1501 if (mlxsw_sp_fid_is_vfid(fid)) {
1502 struct mlxsw_sp_port *mlxsw_sp_vport;
1503
1504 mlxsw_sp_vport =
mlxsw_sp_port_vport_find_by_fid(mlxsw_sp_port,
1505 fid);
1506 if (!mlxsw_sp_vport) {
1507 netdev_err(mlxsw_sp_port->dev, "Failed to
find a matching vPort following FDB notification\n");
1508 goto just_remove;
1509 }
1510 vid = 0;
1511 /* Override the physical port with the vPort. */
1512 mlxsw_sp_port = mlxsw_sp_vport;
1513 } else {
1514 vid = fid;
1515 }
1516
1517do_fdb_op:
1518 err = mlxsw_sp_port_fdb_uc_op(mlxsw_sp, local_port, mac, fid,
1519 adding, true);
1520 if (err) {
1521 if (net_ratelimit())
1522 netdev_err(mlxsw_sp_port->dev, "Failed to
set FDB entry\n");
1523 return;
1524 }
1525
1526 if (!do_notification)
1527 return;
1528 mlxsw_sp_fdb_call_notifiers(mlxsw_sp_port->learning_sync,
1529 adding, mac, vid, mlxsw_sp_port->dev);
1530 return;
1531
1532just_remove:
1533 adding = false;
1534 do_notification = false;
1535 goto do_fdb_op;
1536}
The issue here is that line 1496 implies that mlxsw_sp_port might be
NULL. If this is the case, the execution path jumps to line 1532 and
then to line 1517. All this could end up dereferencing a NULL pointer
at line 1522.
Is there any chance for mlxsw_sp_port to be NULL at line 1496 and, at
the same time, a NULL pointer dereference occurs at line 1522?
I'm trying to figure out if this is a false positive or something that
actually needs to be fixed.
I'd really appreciate any comment on this.
Thank you!
^ permalink raw reply
* Re: [net-mellanox] question about potential null pointer dereference
From: Ido Schimmel @ 2017-05-10 15:51 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Jiri Pirko, netdev, linux-kernel
In-Reply-To: <20170510103659.Horde.a0uOuKuhfTuy1DbqQ378EHK@gator4166.hostgator.com>
On Wed, May 10, 2017 at 10:36:59AM -0500, Gustavo A. R. Silva wrote:
>
> Hello everybody,
>
> While looking into Coverity ID 1350941 I ran into the following piece of
> code at drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c:1483:
>
> 1483static void mlxsw_sp_fdb_notify_mac_process(struct mlxsw_sp *mlxsw_sp,
> 1484 char *sfn_pl, int rec_index,
> 1485 bool adding)
> 1486{
> 1487 struct mlxsw_sp_port *mlxsw_sp_port;
> 1488 char mac[ETH_ALEN];
> 1489 u8 local_port;
> 1490 u16 vid, fid;
> 1491 bool do_notification = true;
> 1492 int err;
> 1493
> 1494 mlxsw_reg_sfn_mac_unpack(sfn_pl, rec_index, mac, &fid,
> &local_port);
> 1495 mlxsw_sp_port = mlxsw_sp->ports[local_port];
> 1496 if (!mlxsw_sp_port) {
> 1497 dev_err_ratelimited(mlxsw_sp->bus_info->dev, "Incorrect
> local port in FDB notification\n");
> 1498 goto just_remove;
> 1499 }
> 1500
> 1501 if (mlxsw_sp_fid_is_vfid(fid)) {
> 1502 struct mlxsw_sp_port *mlxsw_sp_vport;
> 1503
> 1504 mlxsw_sp_vport =
> mlxsw_sp_port_vport_find_by_fid(mlxsw_sp_port,
> 1505 fid);
> 1506 if (!mlxsw_sp_vport) {
> 1507 netdev_err(mlxsw_sp_port->dev, "Failed to find a
> matching vPort following FDB notification\n");
> 1508 goto just_remove;
> 1509 }
> 1510 vid = 0;
> 1511 /* Override the physical port with the vPort. */
> 1512 mlxsw_sp_port = mlxsw_sp_vport;
> 1513 } else {
> 1514 vid = fid;
> 1515 }
> 1516
> 1517do_fdb_op:
> 1518 err = mlxsw_sp_port_fdb_uc_op(mlxsw_sp, local_port, mac, fid,
> 1519 adding, true);
> 1520 if (err) {
> 1521 if (net_ratelimit())
> 1522 netdev_err(mlxsw_sp_port->dev, "Failed to set
> FDB entry\n");
> 1523 return;
> 1524 }
> 1525
> 1526 if (!do_notification)
> 1527 return;
> 1528 mlxsw_sp_fdb_call_notifiers(mlxsw_sp_port->learning_sync,
> 1529 adding, mac, vid, mlxsw_sp_port->dev);
> 1530 return;
> 1531
> 1532just_remove:
> 1533 adding = false;
> 1534 do_notification = false;
> 1535 goto do_fdb_op;
> 1536}
>
>
> The issue here is that line 1496 implies that mlxsw_sp_port might be NULL.
> If this is the case, the execution path jumps to line 1532 and then to line
> 1517. All this could end up dereferencing a NULL pointer at line 1522.
>
> Is there any chance for mlxsw_sp_port to be NULL at line 1496 and, at the
> same time, a NULL pointer dereference occurs at line 1522?
>
> I'm trying to figure out if this is a false positive or something that
> actually needs to be fixed.
In theory, yes, it can happen, but it didn't happen yet. I recently
patched that and now it's in Jiri's queue. I guess he'll send it
tomorrow.
https://github.com/jpirko/linux_mlxsw/commit/4160fb9ad6eeacba7736cfdbf7f52248432c2e89
Thanks for looking into this!
>
> I'd really appreciate any comment on this.
> Thank you!
> --
> Gustavo A. R. Silva
^ permalink raw reply
* Re: bpf pointer alignment validation
From: Daniel Borkmann @ 2017-05-10 15:51 UTC (permalink / raw)
To: David Miller, alexei.starovoitov; +Cc: ast, netdev
In-Reply-To: <20170510.113357.362211347006868605.davem@davemloft.net>
On 05/10/2017 05:33 PM, David Miller wrote:
> From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Date: Tue, 9 May 2017 22:57:37 -0700
>
>> On Tue, May 09, 2017 at 02:32:34PM -0400, David Miller wrote:
>>>
>>> +static u32 calc_align(u32 imm)
>>> +{
>>> + u32 align = 1;
>>> +
>>> + if (!imm)
>>> + return 1U << 31;
>>> +
>>> + while (!(imm & 1)) {
>>> + imm >>= 1;
>>> + align <<= 1;
>>> + }
>>> + return align;
>>> +}
>>
>> same question as in previous reply.
>> Why not to use something like:
>> static u32 calc_align(u32 n)
>> {
>> if (!n)
>> return 1U << 31;
>> return n - ((n - 1) & n);
>> }
>
> Ok.
>
> I did a cursory search and we don't have a generic kernel helper for
> this kind of calculation. I was actually quite surprised, as we
> have one for everything else :-)
>
>> this needs to be tweaked like
>> if (log_level > 1)
>> verbose("%d:", insn_idx);
>> else
>> verbose("\nfrom %d to %d:", prev_insn_idx, insn_idx);
>>
>> otherwise it prints prev_insn_idx which is meaningful
>> only with processing exit and search pruning.
>
> Agreed.
>
>> Nice to see all these comments.
>> I wonder how we can make them automatic in the verifier.
>> Like if verifier can somehow hint the user in such human friendly way
>> about what is happening with the program.
>> Today that's the #1 problem. Most folks complaining
>> that verifier error messages are too hard to understand.
>
> We can put whatever text strings we want into that verifier buffer.
> It is as flexible as netlink extended acks.
>
>> would it make sense to bpf_prog_test_run() it here as well?
>
> We could.
>
>> On x86 not much value, but on sparc we can somehow look for traps?
>> Is there some counter of unaligned traps that we can read and report
>> as error to user space after prog_test_run ?
>
> Unfortunately, no.
>
>> These tests we cannot really run, since they don't do any load/store.
>> I mean more for some future tests. Or some sort of debug warn
>> that there were traps while bpf prog was executed, so the user
>> is alarmed and reports to us, since that would be a bug in verifier
>> align logic?
>
> One thing I could definitely do is add logic to the unaligned trap
> handler to print something special in the logs if we find that we
> are executing BPF code.
>
> The basic structure of the log message can be codified in a generic
> bpf_xxx() helper, which architectures call with the PC and unaligned
> address as arguments.
>
> I was thinking more last night about strict alignment mode for the
> verifier, so that bugs can be spotted on all architectures. But
> it stupidly will not work.
>
> The problem is that x86 and powerpc define NET_IP_ALIGN as 0, so all
> bets are off.
>
> One thing we could do in "strict alignment" verifier mode is pretend
> that NET_IP_ALIGN is 2 in the alignment checks.
Would probably be good nevertheless to have this as a flag for
program loads, which gets then passed through to the verifier to
explicitly enable strict alignment checks.
Might certainly aide developing & testing programs on archs with
efficient unaligned access and later actually running them on archs
that don't have it. (And at minimum, it also helps for checking
the test suite against the verifier.)
^ permalink raw reply
* Re: bpf pointer alignment validation
From: David Miller @ 2017-05-10 15:57 UTC (permalink / raw)
To: daniel; +Cc: alexei.starovoitov, ast, netdev
In-Reply-To: <59133716.7060800@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Wed, 10 May 2017 17:51:50 +0200
> Would probably be good nevertheless to have this as a flag for
> program loads, which gets then passed through to the verifier to
> explicitly enable strict alignment checks.
>
> Might certainly aide developing & testing programs on archs with
> efficient unaligned access and later actually running them on archs
> that don't have it. (And at minimum, it also helps for checking
> the test suite against the verifier.)
Ok, I can implement this flag.
The only question is where to put it? An unused bit in the program
type? :-)
^ permalink raw reply
* Re: [net-mellanox] question about potential null pointer dereference
From: Gustavo A. R. Silva @ 2017-05-10 16:01 UTC (permalink / raw)
To: Ido Schimmel; +Cc: Jiri Pirko, netdev, linux-kernel
In-Reply-To: <20170510155131.GA11944@splinter.mtl.com>
Quoting Ido Schimmel <idosch@mellanox.com>:
> On Wed, May 10, 2017 at 10:36:59AM -0500, Gustavo A. R. Silva wrote:
>>
>> Hello everybody,
>>
>> While looking into Coverity ID 1350941 I ran into the following piece of
>> code at drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c:1483:
>>
>> 1483static void mlxsw_sp_fdb_notify_mac_process(struct mlxsw_sp *mlxsw_sp,
>> 1484 char *sfn_pl, int rec_index,
>> 1485 bool adding)
>> 1486{
>> 1487 struct mlxsw_sp_port *mlxsw_sp_port;
>> 1488 char mac[ETH_ALEN];
>> 1489 u8 local_port;
>> 1490 u16 vid, fid;
>> 1491 bool do_notification = true;
>> 1492 int err;
>> 1493
>> 1494 mlxsw_reg_sfn_mac_unpack(sfn_pl, rec_index, mac, &fid,
>> &local_port);
>> 1495 mlxsw_sp_port = mlxsw_sp->ports[local_port];
>> 1496 if (!mlxsw_sp_port) {
>> 1497 dev_err_ratelimited(mlxsw_sp->bus_info->dev, "Incorrect
>> local port in FDB notification\n");
>> 1498 goto just_remove;
>> 1499 }
>> 1500
>> 1501 if (mlxsw_sp_fid_is_vfid(fid)) {
>> 1502 struct mlxsw_sp_port *mlxsw_sp_vport;
>> 1503
>> 1504 mlxsw_sp_vport =
>> mlxsw_sp_port_vport_find_by_fid(mlxsw_sp_port,
>> 1505 fid);
>> 1506 if (!mlxsw_sp_vport) {
>> 1507 netdev_err(mlxsw_sp_port->dev, "Failed to find a
>> matching vPort following FDB notification\n");
>> 1508 goto just_remove;
>> 1509 }
>> 1510 vid = 0;
>> 1511 /* Override the physical port with the vPort. */
>> 1512 mlxsw_sp_port = mlxsw_sp_vport;
>> 1513 } else {
>> 1514 vid = fid;
>> 1515 }
>> 1516
>> 1517do_fdb_op:
>> 1518 err = mlxsw_sp_port_fdb_uc_op(mlxsw_sp, local_port, mac, fid,
>> 1519 adding, true);
>> 1520 if (err) {
>> 1521 if (net_ratelimit())
>> 1522 netdev_err(mlxsw_sp_port->dev, "Failed to set
>> FDB entry\n");
>> 1523 return;
>> 1524 }
>> 1525
>> 1526 if (!do_notification)
>> 1527 return;
>> 1528 mlxsw_sp_fdb_call_notifiers(mlxsw_sp_port->learning_sync,
>> 1529 adding, mac, vid,
>> mlxsw_sp_port->dev);
>> 1530 return;
>> 1531
>> 1532just_remove:
>> 1533 adding = false;
>> 1534 do_notification = false;
>> 1535 goto do_fdb_op;
>> 1536}
>>
>>
>> The issue here is that line 1496 implies that mlxsw_sp_port might be NULL.
>> If this is the case, the execution path jumps to line 1532 and then to line
>> 1517. All this could end up dereferencing a NULL pointer at line 1522.
>>
>> Is there any chance for mlxsw_sp_port to be NULL at line 1496 and, at the
>> same time, a NULL pointer dereference occurs at line 1522?
>>
>> I'm trying to figure out if this is a false positive or something that
>> actually needs to be fixed.
>
> In theory, yes, it can happen, but it didn't happen yet. I recently
> patched that and now it's in Jiri's queue. I guess he'll send it
> tomorrow.
>
> https://github.com/jpirko/linux_mlxsw/commit/4160fb9ad6eeacba7736cfdbf7f52248432c2e89
>
Great, glad to see it is fixed now.
> Thanks for looking into this!
>
Sure thing, thanks for clarifying. :)
^ permalink raw reply
* Re: bpf pointer alignment validation
From: Alexei Starovoitov @ 2017-05-10 16:15 UTC (permalink / raw)
To: David Miller, daniel; +Cc: alexei.starovoitov, netdev
In-Reply-To: <20170510.115720.1396306489898364855.davem@davemloft.net>
On 5/10/17 8:57 AM, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Wed, 10 May 2017 17:51:50 +0200
>
>> Would probably be good nevertheless to have this as a flag for
>> program loads, which gets then passed through to the verifier to
>> explicitly enable strict alignment checks.
>>
>> Might certainly aide developing & testing programs on archs with
>> efficient unaligned access and later actually running them on archs
>> that don't have it. (And at minimum, it also helps for checking
>> the test suite against the verifier.)
>
> Ok, I can implement this flag.
>
> The only question is where to put it? An unused bit in the program
> type? :-)
just add '__u32 prog_flags' to bpf_attr PROG_LOAD anon union.
There was no need for such flags in the past, hence no flags field
existed. Now it's time.
^ permalink raw reply
* Re: bpf pointer alignment validation
From: Daniel Borkmann @ 2017-05-10 16:21 UTC (permalink / raw)
To: David Miller; +Cc: alexei.starovoitov, ast, netdev
In-Reply-To: <20170510.115720.1396306489898364855.davem@davemloft.net>
On 05/10/2017 05:57 PM, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Wed, 10 May 2017 17:51:50 +0200
>
>> Would probably be good nevertheless to have this as a flag for
>> program loads, which gets then passed through to the verifier to
>> explicitly enable strict alignment checks.
>>
>> Might certainly aide developing & testing programs on archs with
>> efficient unaligned access and later actually running them on archs
>> that don't have it. (And at minimum, it also helps for checking
>> the test suite against the verifier.)
>
> Ok, I can implement this flag.
>
> The only question is where to put it? An unused bit in the program
> type? :-)
See for example 7f677633379b ("bpf: introduce BPF_F_ALLOW_OVERRIDE
flag"). We can add a flags field to the prog loading part of union
bpf_attr; we would need to make sure to update BPF_PROG_LOAD_LAST_FIELD
to the new member, and to reject unknown flags, of course. Then the
syscall will handle compat with older binaries just fine by design,
the main bpf syscall code and CHECK_ATTR() macros will ensure this
(backward compat, and to a limited degree also forward compat).
^ permalink raw reply
* Re: [Patch net] ipv4: restore rt->fi for reference counting
From: Cong Wang @ 2017-05-10 16:32 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Linux Kernel Network Developers, Andrey Konovalov,
Eric Dumazet
In-Reply-To: <1494373805.7796.98.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, May 9, 2017 at 4:50 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-05-09 at 16:35 -0700, Cong Wang wrote:
>
>> All of them take RCU read lock, so, as I explained in the code comment,
>> they all should be fine because of synchronize_net() on unregister path.
>> Do you see anything otherwise?
>
> They might take rcu lock, but compiler is still allowed to read
> fi->fib_dev multiple times, and crashes might happen.
>
> You will need to audit all code and fix it, using proper
> rcu_dereference() or similar code ensuring compiler wont do stupid
> things.
>
Point taken. But without my patch, nh_dev is supposed to be protected
by RCU too, it is freed in a rcu callback and dereferenced like:
struct in_device *in_dev = __in_dev_get_rcu(nh->nh_dev);
^ permalink raw reply
* Re: [Patch net] ipv4: restore rt->fi for reference counting
From: Cong Wang @ 2017-05-10 16:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Linux Kernel Network Developers, Andrey Konovalov,
Eric Dumazet
In-Reply-To: <1494373908.7796.100.camel@edumazet-glaptop3.roam.corp.google.com>
On Tue, May 9, 2017 at 4:51 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-05-09 at 16:35 -0700, Cong Wang wrote:
>
>> This statement is only used to ensure we pass the "dead == fi->fib_nhs"
>> check right below the inner loop, it is fine to keep it without break since
>> fi is not changed in the inner loop.
>>
>
> So the dead++ above wont end up with (dead > fi->fib_nhs) ?
Good point, it could happen, we probably need another boolean to
address this.
^ permalink raw reply
* Re: bpf pointer alignment validation
From: David Miller @ 2017-05-10 16:45 UTC (permalink / raw)
To: daniel; +Cc: alexei.starovoitov, ast, netdev
In-Reply-To: <59133E1E.8060208@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Wed, 10 May 2017 18:21:50 +0200
> On 05/10/2017 05:57 PM, David Miller wrote:
>> From: Daniel Borkmann <daniel@iogearbox.net>
>> Date: Wed, 10 May 2017 17:51:50 +0200
>>
>>> Would probably be good nevertheless to have this as a flag for
>>> program loads, which gets then passed through to the verifier to
>>> explicitly enable strict alignment checks.
>>>
>>> Might certainly aide developing & testing programs on archs with
>>> efficient unaligned access and later actually running them on archs
>>> that don't have it. (And at minimum, it also helps for checking
>>> the test suite against the verifier.)
>>
>> Ok, I can implement this flag.
>>
>> The only question is where to put it? An unused bit in the program
>> type? :-)
>
> See for example 7f677633379b ("bpf: introduce BPF_F_ALLOW_OVERRIDE
> flag"). We can add a flags field to the prog loading part of union
> bpf_attr; we would need to make sure to update
> BPF_PROG_LOAD_LAST_FIELD
> to the new member, and to reject unknown flags, of course. Then the
> syscall will handle compat with older binaries just fine by design,
> the main bpf syscall code and CHECK_ATTR() macros will ensure this
> (backward compat, and to a limited degree also forward compat).
Thanks to both of you for guidance on how to do this properly.
Here is what I have right now, once things look good enough I'll split
it all out into a proper patch series:
1) Add alignment tracking.
2) Add "log_level > 1" per-insn state dumping
3) Add BPF_F_STRICT_ALIGNMENT
4) Add bpf_verify_program() to library
5) Add test_align to selftests
====================
Subject: [PATCH] BPF alignment framework
---
include/linux/bpf_verifier.h | 3 +
include/uapi/linux/bpf.h | 8 +
kernel/bpf/syscall.c | 5 +-
kernel/bpf/verifier.c | 132 ++++++++--
tools/include/uapi/linux/bpf.h | 11 +-
tools/lib/bpf/bpf.c | 22 ++
tools/lib/bpf/bpf.h | 4 +
tools/testing/selftests/bpf/Makefile | 4 +-
tools/testing/selftests/bpf/test_align.c | 417 +++++++++++++++++++++++++++++++
9 files changed, 581 insertions(+), 25 deletions(-)
create mode 100644 tools/testing/selftests/bpf/test_align.c
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 5efb4db..7c6a519 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -40,6 +40,9 @@ struct bpf_reg_state {
*/
s64 min_value;
u64 max_value;
+ u32 min_align;
+ u32 aux_off;
+ u32 aux_off_align;
};
enum bpf_stack_slot_type {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 945a1f5..94dfa9d 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -132,6 +132,13 @@ enum bpf_attach_type {
*/
#define BPF_F_ALLOW_OVERRIDE (1U << 0)
+/* If BPF_F_STRICT_ALIGNMENT is used in BPF_PROG_LOAD command, the
+ * verifier will perform strict alignment checking as if the kernel
+ * has been built with CONFIG_EFFICIENT_UNALIGNED_ACCESS not set,
+ * and NET_IP_ALIGN defined to 2.
+ */
+#define BPF_F_STRICT_ALIGNMENT (1U << 0)
+
#define BPF_PSEUDO_MAP_FD 1
/* flags for BPF_MAP_UPDATE_ELEM command */
@@ -177,6 +184,7 @@ union bpf_attr {
__u32 log_size; /* size of user buffer */
__aligned_u64 log_buf; /* user supplied buffer */
__u32 kern_version; /* checked when prog_type=kprobe */
+ __u32 prog_flags;
};
struct { /* anonymous struct used by BPF_OBJ_* commands */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 13642c7..027053a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -784,7 +784,7 @@ struct bpf_prog *bpf_prog_get_type(u32 ufd, enum bpf_prog_type type)
EXPORT_SYMBOL_GPL(bpf_prog_get_type);
/* last field in 'union bpf_attr' used by this command */
-#define BPF_PROG_LOAD_LAST_FIELD kern_version
+#define BPF_PROG_LOAD_LAST_FIELD prog_flags
static int bpf_prog_load(union bpf_attr *attr)
{
@@ -797,6 +797,9 @@ static int bpf_prog_load(union bpf_attr *attr)
if (CHECK_ATTR(BPF_PROG_LOAD))
return -EINVAL;
+ if (attr->prog_flags & ~BPF_F_STRICT_ALIGNMENT)
+ return -EINVAL;
+
/* copy eBPF program license from user space */
if (strncpy_from_user(license, u64_to_user_ptr(attr->license),
sizeof(license) - 1) < 0)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c2ff608..87ecb4d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -241,6 +241,12 @@ static void print_verifier_state(struct bpf_verifier_state *state)
if (reg->max_value != BPF_REGISTER_MAX_RANGE)
verbose(",max_value=%llu",
(unsigned long long)reg->max_value);
+ if (reg->min_align)
+ verbose(",min_align=%u", reg->min_align);
+ if (reg->aux_off)
+ verbose(",aux_off=%u", reg->aux_off);
+ if (reg->aux_off_align)
+ verbose(",aux_off_align=%u", reg->aux_off_align);
}
for (i = 0; i < MAX_BPF_STACK; i += BPF_REG_SIZE) {
if (state->stack_slot_type[i] == STACK_SPILL)
@@ -455,6 +461,9 @@ static void init_reg_state(struct bpf_reg_state *regs)
regs[i].imm = 0;
regs[i].min_value = BPF_REGISTER_MIN_RANGE;
regs[i].max_value = BPF_REGISTER_MAX_RANGE;
+ regs[i].min_align = 0;
+ regs[i].aux_off = 0;
+ regs[i].aux_off_align = 0;
}
/* frame pointer */
@@ -483,11 +492,17 @@ static void reset_reg_range_values(struct bpf_reg_state *regs, u32 regno)
regs[regno].max_value = BPF_REGISTER_MAX_RANGE;
}
+static void reset_reg_align(struct bpf_reg_state *regs, u32 regno)
+{
+ regs[regno].min_align = 0;
+}
+
static void mark_reg_unknown_value_and_range(struct bpf_reg_state *regs,
u32 regno)
{
mark_reg_unknown_value(regs, regno);
reset_reg_range_values(regs, regno);
+ reset_reg_align(regs, regno);
}
enum reg_arg_type {
@@ -768,15 +783,29 @@ static bool is_pointer_value(struct bpf_verifier_env *env, int regno)
}
static int check_pkt_ptr_alignment(const struct bpf_reg_state *reg,
- int off, int size)
+ int off, int size, bool strict)
{
- if (reg->id && size != 1) {
- verbose("Unknown alignment. Only byte-sized access allowed in packet access.\n");
- return -EACCES;
+ int reg_off;
+
+ /* Byte size accesses are always allowed. */
+ if (!strict || size == 1)
+ return 0;
+
+ reg_off = reg->off;
+ if (reg->id) {
+ if (reg->aux_off_align % size) {
+ verbose("Packet access is only %u byte aligned, %d byte access not allowed\n",
+ reg->aux_off_align, size);
+ return -EACCES;
+ }
+ reg_off += reg->aux_off;
}
- /* skb->data is NET_IP_ALIGN-ed */
- if ((NET_IP_ALIGN + reg->off + off) % size != 0) {
+ /* skb->data is NET_IP_ALIGN-ed, but for strict alignment checking
+ * we force this to 2 which is universally what architectures use
+ * when they don't set CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS.
+ */
+ if (((strict ? 2 : NET_IP_ALIGN) + reg_off + off) % size != 0) {
verbose("misaligned packet access off %d+%d+%d size %d\n",
NET_IP_ALIGN, reg->off, off, size);
return -EACCES;
@@ -786,9 +815,9 @@ static int check_pkt_ptr_alignment(const struct bpf_reg_state *reg,
}
static int check_val_ptr_alignment(const struct bpf_reg_state *reg,
- int size)
+ int size, bool strict)
{
- if (size != 1) {
+ if (strict && size != 1) {
verbose("Unknown alignment. Only byte-sized access allowed in value access.\n");
return -EACCES;
}
@@ -796,16 +825,21 @@ static int check_val_ptr_alignment(const struct bpf_reg_state *reg,
return 0;
}
+static bool strict_alignment;
+
static int check_ptr_alignment(const struct bpf_reg_state *reg,
int off, int size)
{
+ bool strict = strict_alignment;
+
+ if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
+ strict = true;
+
switch (reg->type) {
case PTR_TO_PACKET:
- return IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) ? 0 :
- check_pkt_ptr_alignment(reg, off, size);
+ return check_pkt_ptr_alignment(reg, off, size, strict);
case PTR_TO_MAP_VALUE_ADJ:
- return IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) ? 0 :
- check_val_ptr_alignment(reg, size);
+ return check_val_ptr_alignment(reg, size, strict);
default:
if (off % size != 0) {
verbose("misaligned access off %d size %d\n",
@@ -872,6 +906,8 @@ static int check_mem_access(struct bpf_verifier_env *env, u32 regno, int off,
value_regno);
/* note that reg.[id|off|range] == 0 */
state->regs[value_regno].type = reg_type;
+ state->regs[value_regno].aux_off = 0;
+ state->regs[value_regno].aux_off_align = 0;
}
} else if (reg->type == FRAME_PTR || reg->type == PTR_TO_STACK) {
@@ -1444,6 +1480,8 @@ static int check_packet_ptr_add(struct bpf_verifier_env *env,
*/
dst_reg->off += imm;
} else {
+ bool had_id;
+
if (src_reg->type == PTR_TO_PACKET) {
/* R6=pkt(id=0,off=0,r=62) R7=imm22; r7 += r6 */
tmp_reg = *dst_reg; /* save r7 state */
@@ -1477,14 +1515,23 @@ static int check_packet_ptr_add(struct bpf_verifier_env *env,
src_reg->imm);
return -EACCES;
}
+
+ had_id = (dst_reg->id != 0);
+
/* dst_reg stays as pkt_ptr type and since some positive
* integer value was added to the pointer, increment its 'id'
*/
dst_reg->id = ++env->id_gen;
- /* something was added to pkt_ptr, set range and off to zero */
+ /* something was added to pkt_ptr, set range to zero */
+ dst_reg->aux_off = dst_reg->off;
dst_reg->off = 0;
dst_reg->range = 0;
+ if (had_id)
+ dst_reg->aux_off_align = min(dst_reg->aux_off_align,
+ src_reg->min_align);
+ else
+ dst_reg->aux_off_align = src_reg->min_align;
}
return 0;
}
@@ -1658,6 +1705,13 @@ static void check_reg_overflow(struct bpf_reg_state *reg)
reg->min_value = BPF_REGISTER_MIN_RANGE;
}
+static u32 calc_align(u32 imm)
+{
+ if (!imm)
+ return 1U << 31;
+ return imm - ((imm - 1) & imm);
+}
+
static void adjust_reg_min_max_vals(struct bpf_verifier_env *env,
struct bpf_insn *insn)
{
@@ -1665,8 +1719,10 @@ static void adjust_reg_min_max_vals(struct bpf_verifier_env *env,
s64 min_val = BPF_REGISTER_MIN_RANGE;
u64 max_val = BPF_REGISTER_MAX_RANGE;
u8 opcode = BPF_OP(insn->code);
+ u32 dst_align, src_align;
dst_reg = ®s[insn->dst_reg];
+ src_align = 0;
if (BPF_SRC(insn->code) == BPF_X) {
check_reg_overflow(®s[insn->src_reg]);
min_val = regs[insn->src_reg].min_value;
@@ -1682,18 +1738,25 @@ static void adjust_reg_min_max_vals(struct bpf_verifier_env *env,
regs[insn->src_reg].type != UNKNOWN_VALUE) {
min_val = BPF_REGISTER_MIN_RANGE;
max_val = BPF_REGISTER_MAX_RANGE;
+ src_align = 0;
+ } else {
+ src_align = regs[insn->src_reg].min_align;
}
} else if (insn->imm < BPF_REGISTER_MAX_RANGE &&
(s64)insn->imm > BPF_REGISTER_MIN_RANGE) {
min_val = max_val = insn->imm;
+ src_align = calc_align(insn->imm);
}
+ dst_align = dst_reg->min_align;
+
/* We don't know anything about what was done to this register, mark it
* as unknown.
*/
if (min_val == BPF_REGISTER_MIN_RANGE &&
max_val == BPF_REGISTER_MAX_RANGE) {
reset_reg_range_values(regs, insn->dst_reg);
+ reset_reg_align(regs, insn->dst_reg);
return;
}
@@ -1712,18 +1775,21 @@ static void adjust_reg_min_max_vals(struct bpf_verifier_env *env,
dst_reg->min_value += min_val;
if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
dst_reg->max_value += max_val;
+ dst_reg->min_align = min(src_align, dst_align);
break;
case BPF_SUB:
if (dst_reg->min_value != BPF_REGISTER_MIN_RANGE)
dst_reg->min_value -= min_val;
if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
dst_reg->max_value -= max_val;
+ dst_reg->min_align = min(src_align, dst_align);
break;
case BPF_MUL:
if (dst_reg->min_value != BPF_REGISTER_MIN_RANGE)
dst_reg->min_value *= min_val;
if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
dst_reg->max_value *= max_val;
+ dst_reg->min_align = max(src_align, dst_align);
break;
case BPF_AND:
/* Disallow AND'ing of negative numbers, ain't nobody got time
@@ -1735,17 +1801,23 @@ static void adjust_reg_min_max_vals(struct bpf_verifier_env *env,
else
dst_reg->min_value = 0;
dst_reg->max_value = max_val;
+ dst_reg->min_align = max(src_align, dst_align);
break;
case BPF_LSH:
/* Gotta have special overflow logic here, if we're shifting
* more than MAX_RANGE then just assume we have an invalid
* range.
*/
- if (min_val > ilog2(BPF_REGISTER_MAX_RANGE))
+ if (min_val > ilog2(BPF_REGISTER_MAX_RANGE)) {
dst_reg->min_value = BPF_REGISTER_MIN_RANGE;
- else if (dst_reg->min_value != BPF_REGISTER_MIN_RANGE)
- dst_reg->min_value <<= min_val;
-
+ dst_reg->min_align = 1;
+ } else {
+ if (dst_reg->min_value != BPF_REGISTER_MIN_RANGE)
+ dst_reg->min_value <<= min_val;
+ if (!dst_reg->min_align)
+ dst_reg->min_align = 1;
+ dst_reg->min_align <<= min_val;
+ }
if (max_val > ilog2(BPF_REGISTER_MAX_RANGE))
dst_reg->max_value = BPF_REGISTER_MAX_RANGE;
else if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
@@ -1755,11 +1827,19 @@ static void adjust_reg_min_max_vals(struct bpf_verifier_env *env,
/* RSH by a negative number is undefined, and the BPF_RSH is an
* unsigned shift, so make the appropriate casts.
*/
- if (min_val < 0 || dst_reg->min_value < 0)
+ if (min_val < 0 || dst_reg->min_value < 0) {
dst_reg->min_value = BPF_REGISTER_MIN_RANGE;
- else
+ } else {
dst_reg->min_value =
(u64)(dst_reg->min_value) >> min_val;
+ }
+ if (min_val < 0) {
+ dst_reg->min_align = 1;
+ } else {
+ dst_reg->min_align >>= (u64) min_val;
+ if (!dst_reg->min_align)
+ dst_reg->min_align = 1;
+ }
if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
dst_reg->max_value >>= max_val;
break;
@@ -1861,6 +1941,7 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
regs[insn->dst_reg].imm = insn->imm;
regs[insn->dst_reg].max_value = insn->imm;
regs[insn->dst_reg].min_value = insn->imm;
+ regs[insn->dst_reg].min_align = calc_align(insn->imm);
}
} else if (opcode > BPF_END) {
@@ -2845,8 +2926,12 @@ static int do_check(struct bpf_verifier_env *env)
goto process_bpf_exit;
}
- if (log_level && do_print_state) {
- verbose("\nfrom %d to %d:", prev_insn_idx, insn_idx);
+ if (log_level > 1 || (log_level && do_print_state)) {
+ if (log_level > 1)
+ verbose("%d:", insn_idx);
+ else
+ verbose("\nfrom %d to %d:",
+ prev_insn_idx, insn_idx);
print_verifier_state(&env->cur_state);
do_print_state = false;
}
@@ -3483,6 +3568,10 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
} else {
log_level = 0;
}
+ if (attr->prog_flags & BPF_F_STRICT_ALIGNMENT)
+ strict_alignment = true;
+ else
+ strict_alignment = false;
ret = replace_map_fd_with_map_ptr(env);
if (ret < 0)
@@ -3588,6 +3677,7 @@ int bpf_analyzer(struct bpf_prog *prog, const struct bpf_ext_analyzer_ops *ops,
mutex_lock(&bpf_verifier_lock);
log_level = 0;
+ strict_alignment = false;
env->explored_states = kcalloc(env->prog->len,
sizeof(struct bpf_verifier_state_list *),
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index e553529..94dfa9d 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -132,6 +132,13 @@ enum bpf_attach_type {
*/
#define BPF_F_ALLOW_OVERRIDE (1U << 0)
+/* If BPF_F_STRICT_ALIGNMENT is used in BPF_PROG_LOAD command, the
+ * verifier will perform strict alignment checking as if the kernel
+ * has been built with CONFIG_EFFICIENT_UNALIGNED_ACCESS not set,
+ * and NET_IP_ALIGN defined to 2.
+ */
+#define BPF_F_STRICT_ALIGNMENT (1U << 0)
+
#define BPF_PSEUDO_MAP_FD 1
/* flags for BPF_MAP_UPDATE_ELEM command */
@@ -177,6 +184,7 @@ union bpf_attr {
__u32 log_size; /* size of user buffer */
__aligned_u64 log_buf; /* user supplied buffer */
__u32 kern_version; /* checked when prog_type=kprobe */
+ __u32 prog_flags;
};
struct { /* anonymous struct used by BPF_OBJ_* commands */
@@ -481,8 +489,7 @@ union bpf_attr {
* u32 bpf_get_socket_uid(skb)
* Get the owner uid of the socket stored inside sk_buff.
* @skb: pointer to skb
- * Return: uid of the socket owner on success or 0 if the socket pointer
- * inside sk_buff is NULL
+ * Return: uid of the socket owner on success or overflowuid if failed.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 4fe444b80..6e17898 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -117,6 +117,28 @@ int bpf_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
return sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
}
+int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns,
+ size_t insns_cnt, int strict_alignment,
+ const char *license, __u32 kern_version,
+ char *log_buf, size_t log_buf_sz)
+{
+ union bpf_attr attr;
+
+ bzero(&attr, sizeof(attr));
+ attr.prog_type = type;
+ attr.insn_cnt = (__u32)insns_cnt;
+ attr.insns = ptr_to_u64(insns);
+ attr.license = ptr_to_u64(license);
+ attr.log_buf = ptr_to_u64(log_buf);
+ attr.log_size = log_buf_sz;
+ attr.log_level = 2;
+ log_buf[0] = 0;
+ attr.kern_version = kern_version;
+ attr.prog_flags = strict_alignment ? BPF_F_STRICT_ALIGNMENT : 0;
+
+ return sys_bpf(BPF_PROG_LOAD, &attr, sizeof(attr));
+}
+
int bpf_map_update_elem(int fd, const void *key, const void *value,
__u64 flags)
{
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index edb4dae..972bd83 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -35,6 +35,10 @@ int bpf_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
size_t insns_cnt, const char *license,
__u32 kern_version, char *log_buf,
size_t log_buf_sz);
+int bpf_verify_program(enum bpf_prog_type type, const struct bpf_insn *insns,
+ size_t insns_cnt, int strict_alignment,
+ const char *license, __u32 kern_version,
+ char *log_buf, size_t log_buf_sz);
int bpf_map_update_elem(int fd, const void *key, const void *value,
__u64 flags);
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 91edd05..1e1cde1 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -11,7 +11,8 @@ endif
CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) -I../../../include
LDLIBS += -lcap -lelf
-TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs
+TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
+ test_align
TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o
diff --git a/tools/testing/selftests/bpf/test_align.c b/tools/testing/selftests/bpf/test_align.c
new file mode 100644
index 0000000..9dd9794
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_align.c
@@ -0,0 +1,417 @@
+#include <asm/types.h>
+#include <linux/types.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <stddef.h>
+#include <stdbool.h>
+
+#include <linux/unistd.h>
+#include <linux/filter.h>
+#include <linux/bpf_perf_event.h>
+#include <linux/bpf.h>
+
+#include <bpf/bpf.h>
+
+#include "../../../include/linux/filter.h"
+
+#ifndef ARRAY_SIZE
+# define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
+#define MAX_INSNS 512
+#define MAX_MATCHES 16
+
+struct bpf_align_test {
+ const char *descr;
+ struct bpf_insn insns[MAX_INSNS];
+ enum {
+ UNDEF,
+ ACCEPT,
+ REJECT
+ } result;
+ enum bpf_prog_type prog_type;
+ const char *matches[MAX_MATCHES];
+};
+
+static struct bpf_align_test tests[] = {
+ {
+ .descr = "mov",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_3, 2),
+ BPF_MOV64_IMM(BPF_REG_3, 4),
+ BPF_MOV64_IMM(BPF_REG_3, 8),
+ BPF_MOV64_IMM(BPF_REG_3, 16),
+ BPF_MOV64_IMM(BPF_REG_3, 32),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .matches = {
+ "1: R1=ctx R3=imm2,min_value=2,max_value=2,min_align=2 R10=fp",
+ "2: R1=ctx R3=imm4,min_value=4,max_value=4,min_align=4 R10=fp",
+ "3: R1=ctx R3=imm8,min_value=8,max_value=8,min_align=8 R10=fp",
+ "4: R1=ctx R3=imm16,min_value=16,max_value=16,min_align=16 R10=fp",
+ "5: R1=ctx R3=imm32,min_value=32,max_value=32,min_align=32 R10=fp",
+ },
+ },
+ {
+ .descr = "shift",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_3, 1),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_3, 1),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_3, 1),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_3, 1),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_3, 1),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_3, 4),
+ BPF_MOV64_IMM(BPF_REG_4, 32),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_4, 1),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_4, 1),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_4, 1),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_4, 1),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .matches = {
+ "1: R1=ctx R3=imm1,min_value=1,max_value=1,min_align=1 R10=fp",
+ "2: R1=ctx R3=imm2,min_value=2,max_value=2,min_align=2 R10=fp",
+ "3: R1=ctx R3=imm4,min_value=4,max_value=4,min_align=4 R10=fp",
+ "4: R1=ctx R3=imm8,min_value=8,max_value=8,min_align=8 R10=fp",
+ "5: R1=ctx R3=imm16,min_value=16,max_value=16,min_align=16 R10=fp",
+ "6: R1=ctx R3=imm1,min_value=1,max_value=1,min_align=1 R10=fp",
+ "7: R1=ctx R3=imm1,min_value=1,max_value=1,min_align=1 R4=imm32,min_value=32,max_value=32,min_align=32 R10=fp",
+ "8: R1=ctx R3=imm1,min_value=1,max_value=1,min_align=1 R4=imm16,min_value=16,max_value=16,min_align=16 R10=fp",
+ "9: R1=ctx R3=imm1,min_value=1,max_value=1,min_align=1 R4=imm8,min_value=8,max_value=8,min_align=8 R10=fp",
+ "10: R1=ctx R3=imm1,min_value=1,max_value=1,min_align=1 R4=imm4,min_value=4,max_value=4,min_align=4 R10=fp",
+ "11: R1=ctx R3=imm1,min_value=1,max_value=1,min_align=1 R4=imm2,min_value=2,max_value=2,min_align=2 R10=fp",
+ },
+ },
+ {
+ .descr = "addsub",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_3, 4),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, 4),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, 2),
+ BPF_MOV64_IMM(BPF_REG_4, 8),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, 4),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .matches = {
+ "1: R1=ctx R3=imm4,min_value=4,max_value=4,min_align=4 R10=fp",
+ "2: R1=ctx R3=imm8,min_value=8,max_value=8,min_align=4 R10=fp",
+ "3: R1=ctx R3=imm10,min_value=10,max_value=10,min_align=2 R10=fp",
+ "4: R1=ctx R3=imm10,min_value=10,max_value=10,min_align=2 R4=imm8,min_value=8,max_value=8,min_align=8 R10=fp",
+ "5: R1=ctx R3=imm10,min_value=10,max_value=10,min_align=2 R4=imm12,min_value=12,max_value=12,min_align=4 R10=fp",
+ "6: R1=ctx R3=imm10,min_value=10,max_value=10,min_align=2 R4=imm14,min_value=14,max_value=14,min_align=2 R10=fp",
+ },
+ },
+ {
+ .descr = "mul",
+ .insns = {
+ BPF_MOV64_IMM(BPF_REG_3, 7),
+ BPF_ALU64_IMM(BPF_MUL, BPF_REG_3, 1),
+ BPF_ALU64_IMM(BPF_MUL, BPF_REG_3, 2),
+ BPF_ALU64_IMM(BPF_MUL, BPF_REG_3, 4),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .matches = {
+ "1: R1=ctx R3=imm7,min_value=7,max_value=7,min_align=1 R10=fp",
+ "2: R1=ctx R3=imm7,min_value=7,max_value=7,min_align=1 R10=fp",
+ "3: R1=ctx R3=imm14,min_value=14,max_value=14,min_align=2 R10=fp",
+ "4: R1=ctx R3=imm56,min_value=56,max_value=56,min_align=4 R10=fp",
+ },
+ },
+
+#define PREP_PKT_POINTERS \
+ BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1, \
+ offsetof(struct __sk_buff, data)), \
+ BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1, \
+ offsetof(struct __sk_buff, data_end))
+
+#define LOAD_UNKNOWN(DST_REG) \
+ PREP_PKT_POINTERS, \
+ BPF_MOV64_REG(BPF_REG_0, BPF_REG_2), \
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 8), \
+ BPF_JMP_REG(BPF_JGE, BPF_REG_3, BPF_REG_0, 1), \
+ BPF_EXIT_INSN(), \
+ BPF_LDX_MEM(BPF_B, DST_REG, BPF_REG_2, 0)
+
+ {
+ .descr = "unknown shift",
+ .insns = {
+ LOAD_UNKNOWN(BPF_REG_3),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_3, 1),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_3, 1),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_3, 1),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_3, 1),
+ LOAD_UNKNOWN(BPF_REG_4),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_4, 5),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_4, 1),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_4, 1),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_4, 1),
+ BPF_ALU64_IMM(BPF_RSH, BPF_REG_4, 1),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .matches = {
+ "7: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R10=fp",
+ "8: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv55,min_align=2 R10=fp",
+ "9: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv54,min_align=4 R10=fp",
+ "10: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv53,min_align=8 R10=fp",
+ "11: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv52,min_align=16 R10=fp",
+ "18: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R4=inv56 R10=fp",
+ "19: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R4=inv51,min_align=32 R10=fp",
+ "20: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R4=inv52,min_align=16 R10=fp",
+ "21: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R4=inv53,min_align=8 R10=fp",
+ "22: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R4=inv54,min_align=4 R10=fp",
+ "23: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R4=inv55,min_align=2 R10=fp",
+ },
+ },
+ {
+ .descr = "unknown mul",
+ .insns = {
+ LOAD_UNKNOWN(BPF_REG_3),
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_3),
+ BPF_ALU64_IMM(BPF_MUL, BPF_REG_4, 1),
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_3),
+ BPF_ALU64_IMM(BPF_MUL, BPF_REG_4, 2),
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_3),
+ BPF_ALU64_IMM(BPF_MUL, BPF_REG_4, 4),
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_3),
+ BPF_ALU64_IMM(BPF_MUL, BPF_REG_4, 8),
+ BPF_ALU64_IMM(BPF_MUL, BPF_REG_4, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .matches = {
+ "7: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R10=fp",
+ "8: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R4=inv56 R10=fp",
+ "9: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R4=inv55,min_align=1 R10=fp",
+ "10: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R4=inv56 R10=fp",
+ "11: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R4=inv54,min_align=2 R10=fp",
+ "12: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R4=inv56 R10=fp",
+ "13: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R4=inv53,min_align=4 R10=fp",
+ "14: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R4=inv56 R10=fp",
+ "15: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R4=inv52,min_align=8 R10=fp",
+ "16: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=inv56 R4=inv50,min_align=8 R10=fp"
+ },
+ },
+ {
+ .descr = "packet const offset",
+ .insns = {
+ PREP_PKT_POINTERS,
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_2),
+
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+
+ /* Skip over ethernet header. */
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 14),
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_5),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, 4),
+ BPF_JMP_REG(BPF_JGE, BPF_REG_3, BPF_REG_4, 1),
+ BPF_EXIT_INSN(),
+
+ BPF_LDX_MEM(BPF_B, BPF_REG_4, BPF_REG_5, 0),
+ BPF_LDX_MEM(BPF_B, BPF_REG_4, BPF_REG_5, 1),
+ BPF_LDX_MEM(BPF_B, BPF_REG_4, BPF_REG_5, 2),
+ BPF_LDX_MEM(BPF_B, BPF_REG_4, BPF_REG_5, 3),
+ BPF_LDX_MEM(BPF_H, BPF_REG_4, BPF_REG_5, 0),
+ BPF_LDX_MEM(BPF_H, BPF_REG_4, BPF_REG_5, 2),
+ BPF_LDX_MEM(BPF_W, BPF_REG_4, BPF_REG_5, 0),
+
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .matches = {
+ "4: R0=imm0,min_value=0,max_value=0,min_align=2147483648 R1=ctx R2=pkt(id=0,off=0,r=0) R3=pkt_end R5=pkt(id=0,off=0,r=0) R10=fp",
+ "5: R0=imm0,min_value=0,max_value=0,min_align=2147483648 R1=ctx R2=pkt(id=0,off=0,r=0) R3=pkt_end R5=pkt(id=0,off=14,r=0) R10=fp",
+ "6: R0=imm0,min_value=0,max_value=0,min_align=2147483648 R1=ctx R2=pkt(id=0,off=0,r=0) R3=pkt_end R4=pkt(id=0,off=14,r=0) R5=pkt(id=0,off=14,r=0) R10=fp",
+ "10: R0=imm0,min_value=0,max_value=0,min_align=2147483648 R1=ctx R2=pkt(id=0,off=0,r=18) R3=pkt_end R4=inv56 R5=pkt(id=0,off=14,r=18) R10=fp",
+ "14: R0=imm0,min_value=0,max_value=0,min_align=2147483648 R1=ctx R2=pkt(id=0,off=0,r=18) R3=pkt_end R4=inv48 R5=pkt(id=0,off=14,r=18) R10=fp",
+ "15: R0=imm0,min_value=0,max_value=0,min_align=2147483648 R1=ctx R2=pkt(id=0,off=0,r=18) R3=pkt_end R4=inv48 R5=pkt(id=0,off=14,r=18) R10=fp",
+ },
+ },
+ {
+ .descr = "packet variable offset",
+ .insns = {
+ LOAD_UNKNOWN(BPF_REG_6),
+ BPF_ALU64_IMM(BPF_LSH, BPF_REG_6, 2),
+
+ /* First, add a constant to the R5 packet pointer,
+ * then a variable with a known alignment.
+ */
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_2),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 14),
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_5, BPF_REG_6),
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_5),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, 4),
+ BPF_JMP_REG(BPF_JGE, BPF_REG_3, BPF_REG_4, 1),
+ BPF_EXIT_INSN(),
+ BPF_LDX_MEM(BPF_W, BPF_REG_4, BPF_REG_5, 0),
+
+ /* Now, test in the other direction. Adding first
+ * the variable offset to R5, then the constant.
+ */
+ BPF_MOV64_REG(BPF_REG_5, BPF_REG_2),
+ BPF_ALU64_REG(BPF_ADD, BPF_REG_5, BPF_REG_6),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 14),
+ BPF_MOV64_REG(BPF_REG_4, BPF_REG_5),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, 4),
+ BPF_JMP_REG(BPF_JGE, BPF_REG_3, BPF_REG_4, 1),
+ BPF_EXIT_INSN(),
+ BPF_LDX_MEM(BPF_W, BPF_REG_4, BPF_REG_5, 0),
+
+ BPF_MOV64_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+ .matches = {
+ /* Calculated offset in R6 has unknown value, but known
+ * alignment of 4.
+ */
+ "8: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R6=inv54,min_align=4 R10=fp",
+
+ /* Offset is added to packet pointer R5, resulting in known
+ * auxiliary alignment and offset.
+ */
+ "11: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R5=pkt(id=1,off=0,r=0),aux_off=14,aux_off_align=4 R6=inv54,min_align=4 R10=fp",
+
+ /* At the time the word size load is performed from R5,
+ * it's total offset is NET_IP_ALIGN + reg->off (0) +
+ * reg->aux_off (14) which is 16. Then the variable
+ * offset is considered using reg->aux_off_align which
+ * is 4 and meets the load's requirements.
+ */
+ "15: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R4=pkt(id=1,off=4,r=4),aux_off=14,aux_off_align=4 R5=pkt(id=1,off=0,r=4),aux_off=14,aux_off_align=4 R6=inv54,min_align=4 R10=fp",
+
+
+ /* Variable offset is added to R5 packet pointer,
+ * resulting in auxiliary alignment of 4.
+ */
+ "18: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R4=inv,aux_off=14,aux_off_align=4 R5=pkt(id=2,off=0,r=0),aux_off_align=4 R6=inv54,min_align=4 R10=fp",
+
+ /* Constant offset is added to R5, resulting in
+ * reg->off of 14.
+ */
+ "19: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R4=inv,aux_off=14,aux_off_align=4 R5=pkt(id=2,off=14,r=0),aux_off_align=4 R6=inv54,min_align=4 R10=fp",
+
+ /* At the time the word size load is performed from R5,
+ * it's total offset is NET_IP_ALIGN + reg->off (14) which
+ * is 16. Then the variable offset is considered using
+ * reg->aux_off_align which is 4 and meets the load's
+ * requirements.
+ */
+ "23: R0=pkt(id=0,off=8,r=8) R1=ctx R2=pkt(id=0,off=0,r=8) R3=pkt_end R4=pkt(id=2,off=18,r=18),aux_off_align=4 R5=pkt(id=2,off=14,r=18),aux_off_align=4 R6=inv54,min_align=4 R10=fp",
+ },
+ },
+};
+
+static int probe_filter_length(const struct bpf_insn *fp)
+{
+ int len;
+
+ for (len = MAX_INSNS - 1; len > 0; --len)
+ if (fp[len].code != 0 || fp[len].imm != 0)
+ break;
+ return len + 1;
+}
+
+static char bpf_vlog[32768];
+
+static int do_test_single(struct bpf_align_test *test)
+{
+ struct bpf_insn *prog = test->insns;
+ int prog_type = test->prog_type;
+ int prog_len, i;
+ int fd_prog;
+ int ret;
+
+ prog_len = probe_filter_length(prog);
+ fd_prog = bpf_verify_program(prog_type ? : BPF_PROG_TYPE_SOCKET_FILTER,
+ prog, prog_len, 1, "GPL", 0,
+ bpf_vlog, sizeof(bpf_vlog));
+ if (fd_prog < 0) {
+ printf("Failed to load program.\n");
+ printf("%s", bpf_vlog);
+ ret = 1;
+ } else {
+ ret = 0;
+ for (i = 0; i < MAX_MATCHES; i++) {
+ const char *t, *m = test->matches[i];
+
+ if (!m)
+ break;
+ t = strstr(bpf_vlog, m);
+ if (!t) {
+ printf("Failed to find match: %s\n", m);
+ ret = 1;
+ printf("%s", bpf_vlog);
+ break;
+ }
+ }
+ /* printf("%s", bpf_vlog); */
+ close(fd_prog);
+ }
+ return ret;
+}
+
+static int do_test(unsigned int from, unsigned int to)
+{
+ int all_pass = 0;
+ int all_fail = 0;
+ unsigned int i;
+
+ for (i = from; i < to; i++) {
+ struct bpf_align_test *test = &tests[i];
+ int fail;
+
+ printf("Test %3d: %s ... ",
+ i, test->descr);
+ fail = do_test_single(test);
+ if (fail) {
+ all_fail++;
+ printf("FAIL\n");
+ } else {
+ all_pass++;
+ printf("PASS\n");
+ }
+ }
+ printf("Results: %d pass %d fail\n",
+ all_pass, all_fail);
+ return 0;
+}
+
+int main(int argc, char **argv)
+{
+ unsigned int from = 0, to = ARRAY_SIZE(tests);
+
+ if (argc == 3) {
+ unsigned int l = atoi(argv[argc - 2]);
+ unsigned int u = atoi(argv[argc - 1]);
+
+ if (l < to && u < to) {
+ from = l;
+ to = u + 1;
+ }
+ } else if (argc == 2) {
+ unsigned int t = atoi(argv[argc - 1]);
+
+ if (t < to) {
+ from = t;
+ to = t + 1;
+ }
+ }
+ return do_test(from, to);
+}
--
2.1.2.532.g19b5d50
^ permalink raw reply related
* Re: [PATCH net 2/3] net/mlx4_en: Avoid adding steering rules with invalid ring
From: Or Gerlitz @ 2017-05-10 16:55 UTC (permalink / raw)
To: David S. Miller
Cc: Linux Netdev List, Eran Ben Elisha, Talat Batheesh, Tariq Toukan
In-Reply-To: <1494330324-11752-3-git-send-email-tariqt@mellanox.com>
On Tue, May 9, 2017 at 2:45 PM, Tariq Toukan <tariqt@mellanox.com> wrote:
> From: Talat Batheesh <talatb@mellanox.com>
>
> Inserting steering rules with illegal ring is an invalid operation, block it.
Hi Dave,
I realized today that the patch introduced a regression, Tariq will
see if to revert it as a whole or fix the regression.
Just wanted to drop you a note and to make sure you don't further
carry it to -stable,
xxit happens :(
Or.
^ permalink raw reply
* Re: [Patch net] ipv4: restore rt->fi for reference counting
From: Cong Wang @ 2017-05-10 17:00 UTC (permalink / raw)
To: Julian Anastasov
Cc: Eric Dumazet, David Miller, Linux Kernel Network Developers,
Andrey Konovalov, Eric Dumazet
In-Reply-To: <alpine.LFD.2.20.1705100937330.1938@ja.home.ssi.bg>
On Wed, May 10, 2017 at 12:38 AM, Julian Anastasov <ja@ssi.bg> wrote:
>
> Hello,
>
> On Tue, 9 May 2017, Cong Wang wrote:
>
>> > Also setting nexthop_nh->nh_dev to NULL looks quite dangerous
>> >
>> > We have plenty of sites doing :
>> >
>> > if (fi->fib_dev)
>> > x = fi->fib_dev->field
>> >
>> > fib_route_seq_show() is one example.
>> >
>>
>> All of them take RCU read lock, so, as I explained in the code comment,
>> they all should be fine because of synchronize_net() on unregister path.
>> Do you see anything otherwise?
>
> During NETDEV_UNREGISTER packets for dev should not
> be flying but packets for other devs can walk the nexthops
> for multipath routes. It is the rcu_barrier before
> NETDEV_UNREGISTER_FINAL that allows nh_dev to be set to NULL
> during this grace period but there are many places to fix that
> assume nh_dev!=NULL.
Excellent point. Unfortunately NETDEV_UNREGISTER_FINAL is not
yet captured by the fib event notifier.
I think readers are still safe to assume nh_dev!=NULL since we wait
for existing readers, readers coming later can't see it any more. Or
am I missing anything?
>
> But why we leak routes? Is there some place that holds
> routes without listening for NETDEV_UNREGISTER? On fib_flush
> the infos are unlinked from trees, so after a grace period packets
> should not see/hold such infos. If we hold routes somewhere for
> long time, problem can happen also for routes with single nexthop.
My theory is that dst which holds a refcnt to fib_info (of course, after
this patch) is moved in GC after NETDEV_UNREGISTER but still
referenced somewhere, it therefore holds these nh_dev's, which
in turn hold back to the netdevice which is being unregistered, thus
Eric saw these refcnt leak warnings.
I am not sure if sitting in GC for a long time is problematic or not,
but from the code where we transfer dst->dev to loopback_dev,
it seems this is expected otherwise no need to transfer? But I don't
dig the history though.
Thanks.
^ 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