* Re: KASAN: use-after-free Read in sctp_packet_transmit
From: Marcelo Ricardo Leitner @ 2018-01-06 17:02 UTC (permalink / raw)
To: syzbot
Cc: davem, linux-kernel, linux-sctp, netdev, nhorman, syzkaller-bugs,
vyasevich
In-Reply-To: <94eb2c1fcf4cf899b405620eaa66@google.com>
On Fri, Jan 05, 2018 at 02:07:01PM -0800, syzbot wrote:
> Hello,
>
> syzkaller hit the following crash on
> 8a4816cad00bf14642f0ed6043b32d29a05006ce
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> Unfortunately, I don't have any reproducer for this bug yet.
How can we tell if there wasn't any list corruption messages before
the panic?
Marcelo
^ permalink raw reply
* Re: [PATCH 01/18] asm-generic/barrier: add generic nospec helpers
From: Mark Rutland @ 2018-01-06 17:08 UTC (permalink / raw)
To: Dan Williams
Cc: Linus Torvalds, Linux Kernel Mailing List, linux-arch,
Peter Zijlstra, Network Development, Will Deacon,
Greg Kroah-Hartman, Thomas Gleixner, Alan Cox
In-Reply-To: <CAPcyv4jAt5HTc5iB7603tnvDqdFZ1+JhpekekcZwuVR3g6jjVw@mail.gmail.com>
On Fri, Jan 05, 2018 at 09:23:06PM -0800, Dan Williams wrote:
> On Fri, Jan 5, 2018 at 6:55 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Fri, Jan 5, 2018 at 5:09 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> >> +#ifndef nospec_ptr
> >> +#define nospec_ptr(ptr, lo, hi) \
> >
> > Do we actually want this horrible interface?
> >
> > It just causes the compiler - or inline asm - to generate worse code,
> > because it needs to compare against both high and low limits.
> >
> > Basically all users are arrays that are zero-based, and where a
> > comparison against the high _index_ limit would be sufficient.
> >
> > But the way this is all designed, it's literally designed for bad code
> > generation for the unusual case, and the usual array case is written
> > in the form of the unusual and wrong non-array case. That really seems
> > excessively stupid.
>
> Yes, it appears we can kill nospec_ptr() and move nospec_array_ptr()
> to assume 0 based arrays rather than use nospec_ptr.
Sounds good to me; I can respin the arm/arm64 implementations accordingly.
We can always revisit that if we have non-array cases that need to be covered.
Thanks,
Mark.
^ permalink raw reply
* Re: KASAN: use-after-free Read in sctp_packet_transmit
From: Dmitry Vyukov @ 2018-01-06 17:09 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: syzbot, David Miller, LKML, linux-sctp, netdev, Neil Horman,
syzkaller-bugs, Vladislav Yasevich
In-Reply-To: <20180106170254.GH725@localhost.localdomain>
On Sat, Jan 6, 2018 at 6:02 PM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Fri, Jan 05, 2018 at 02:07:01PM -0800, syzbot wrote:
>> Hello,
>>
>> syzkaller hit the following crash on
>> 8a4816cad00bf14642f0ed6043b32d29a05006ce
>> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/master
>> compiler: gcc (GCC) 7.1.1 20170620
>> .config is attached
>> Raw console output is attached.
>> Unfortunately, I don't have any reproducer for this bug yet.
>
> How can we tell if there wasn't any list corruption messages before
> the panic?
Hi Marcelo,
syzbot always gives reports on non-tainted kernels. So, no, there was
nothing bad before this.
^ permalink raw reply
* Re: [patch net-next v6 01/11] net: sched: introduce support for multiple filter chain pointers registration
From: Jiri Pirko @ 2018-01-06 17:11 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, saeedm, matanb, leonro,
idosch, jakub.kicinski, simon.horman, pieter.jansenvanvuuren,
john.hurley, alexander.h.duyck, ogerlitz, john.fastabend, daniel,
dsahern
In-Reply-To: <20180105230929.5645-2-jiri@resnulli.us>
Sat, Jan 06, 2018 at 12:09:19AM CET, jiri@resnulli.us wrote:
>From: Jiri Pirko <jiri@mellanox.com>
>
>So far, there was possible only to register a single filter chain
>pointer to block->chain[0]. However, when the blocks will get shareable,
>we need to allow multiple filter chain pointers registration.
>
>Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>---
[...]
>+static int tcf_block_insert(struct tcf_block *block, struct net *net,
>+ u32 block_index, struct netlink_ext_ack *extack)
>+{
>+ struct tcf_net *tn = net_generic(net, tcf_net_id);
>+ int idr_start;
>+ int idr_end;
>+ int index;
>+
>+ if (block_index >= INT_MAX) {
>+ NL_SET_ERR_MSG(extack, "Invalid block index value (>= INT_MAX)");
>+ return -EINVAL;
>+ }
>+ idr_start = block_index ? block_index : 1;
>+ idr_end = block_index ? block_index + 1 : INT_MAX;
>+
>+ index = idr_alloc(&tn->idr, block, idr_start, idr_end, GFP_KERNEL);
Oh, I have to do idr_alloc_ext
^ permalink raw reply
* Re: KASAN: use-after-free Read in sctp_packet_transmit
From: Marcelo @ 2018-01-06 17:16 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: syzbot, David Miller, LKML, linux-sctp, netdev, Neil Horman,
syzkaller-bugs, Vladislav Yasevich
In-Reply-To: <CACT4Y+Y_kWEqfqSEiq_yOk7_nm5MuejNbORcfQgspzKHC3aTng@mail.gmail.com>
Em 6 de janeiro de 2018 15:09:45 BRST, Dmitry Vyukov <dvyukov@google.com> escreveu:
>On Sat, Jan 6, 2018 at 6:02 PM, Marcelo Ricardo Leitner
><marcelo.leitner@gmail.com> wrote:
>> On Fri, Jan 05, 2018 at 02:07:01PM -0800, syzbot wrote:
>>> Hello,
>>>
>>> syzkaller hit the following crash on
>>> 8a4816cad00bf14642f0ed6043b32d29a05006ce
>>>
>git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/master
>>> compiler: gcc (GCC) 7.1.1 20170620
>>> .config is attached
>>> Raw console output is attached.
>>> Unfortunately, I don't have any reproducer for this bug yet.
>>
>> How can we tell if there wasn't any list corruption messages before
>> the panic?
>
>Hi Marcelo,
>
>syzbot always gives reports on non-tainted kernels. So, no, there was
>nothing bad before this.
OK. Thanks
--
Enviado de meu dispositivo Android com K-9 mail. Desculpe-me pela brevidade.
^ permalink raw reply
* Re: [PATCH 12/18] Thermal/int340x: prevent bounds-check bypass via speculative execution
From: Srinivas Pandruvada @ 2018-01-06 17:24 UTC (permalink / raw)
To: Dan Williams
Cc: Linux Kernel Mailing List, linux-arch, Greg KH, Peter Zijlstra,
Netdev, Eduardo Valentin, Zhang Rui, Linus Torvalds,
Thomas Gleixner, Elena Reshetova, Alan Cox
In-Reply-To: <CAPcyv4iNJrcKBiN+3=ZZ1MHU1PYbt4rLDLgWZY5Kc+O8regDYw@mail.gmail.com>
On Fri, 2018-01-05 at 17:57 -0800, Dan Williams wrote:
> On Fri, Jan 5, 2018 at 5:53 PM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> >
> > On Fri, 2018-01-05 at 17:10 -0800, Dan Williams wrote:
> > >
> > > Static analysis reports that 'trip' may be a user controlled
> > > value
> > > that
> > > is used as a data dependency to read '*temp' from the 'd-
> > > >aux_trips'
> > > array. In order to avoid potential leaks of kernel memory
> > > values,
> > > block
> > > speculative execution of the instruction stream that could issue
> > > reads
> > > based on an invalid value of '*temp'.
> > Not against the change as this is in a very slow path. But the trip
> > is
> > not an arbitrary value which user can enter.
> >
> > This trip value is the one of the sysfs attribute in thermal zone.
> > For
> > example
> >
> > # cd /sys/class/thermal/thermal_zone1
> > # ls trip_point_?_temp
> > trip_point_0_temp trip_point_1_temp trip_point_2_temp trip_point
> > _3_t
> > emp trip_point_4_temp trip_point_5_temp trip_point_6_temp
> >
> > Here the "trip" is one of the above trip_point_*_temp. So in this
> > case
> > it can be from 0 to 6 as user can't do
> > # cat trip_point_7_temp
> > as there is no sysfs attribute for trip_point_7_temp.
> >
> > The actual "trip" was obtained in thermal core via
> >
> > if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip) !=
> > 1)
> > return -EINVAL;
> >
> > Thanks,
> > Srinivas
> Ah, great, thanks. So do we even need the bounds check at that point?
We are not bound checking but the way we identify type of the trip.
Based on ACPI support we order trips:
- Aux trips max_count = aux_trip_nr
- One Critical trip
- One Hot trip
- One passive trip
- Rest all trips are active trips
So in the above example if d->aux_trip_nr is 1 then trip_point_0_temp
read/write is for aux trip. If d->aux_trip_nr is 0 then it can be any
other non aux trip.
BUT I am not still up to date with these attacks. Not sure about the
perimeter of user controlled value. It is a user controlled but limited
by the sysfs attributes. So I will test this patch and let you know if
there are any issues.
Thanks,
Srinivas
^ permalink raw reply
* Re: KASAN: use-after-free Read in sctp_packet_transmit
From: Denis Kirjanov @ 2018-01-06 17:29 UTC (permalink / raw)
To: syzbot
Cc: davem, linux-kernel, linux-sctp, netdev, nhorman, syzkaller-bugs,
vyasevich
In-Reply-To: <94eb2c1fcf4cf899b405620eaa66@google.com>
On 1/5/18, syzbot <syzbot+5adcca18fca253b4cb15@syzkaller.appspotmail.com> wrote:
> Hello,
>
> syzkaller hit the following crash on
> 8a4816cad00bf14642f0ed6043b32d29a05006ce
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> Unfortunately, I don't have any reproducer for this bug yet.
>
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+5adcca18fca253b4cb15@syzkaller.appspotmail.com
> It will help syzbot understand when the bug is fixed. See footer for
> details.
> If you forward the report, please keep this part and the footer.
>
> ==================================================================
> BUG: KASAN: use-after-free in sctp_packet_transmit+0x3505/0x3750
> net/sctp/output.c:643
> Read of size 8 at addr ffff8801bda9fb80 by task modprobe/23740
>
This can be related to the following corruption during send:
#6 [ffff8805945ff940] invalid_op at ffffffff8100c15b
[exception RIP: sctp_chunk_put+91]
RIP: ffffffffa039db3b RSP: ffff8805945ff9f8 RFLAGS: 00010212
RAX: ffff8808b025cb01 RBX: ffff880dbb1b0d80 RCX: ffff8805945ff818
RDX: 0000000000000020 RSI: ffff8809a84746d8 RDI: ffff880dbb1b0d80
RBP: ffff8805945ffa08 R8: ffff880dbb13a0c0 R9: 0000000000000000
R10: ffff880000023820 R11: 0000000000000000 R12: ffff880dbb1b0d80
R13: 0000000000000000 R14: ffff8808b025cb80 R15: 0000000000000000
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
#7 [ffff8805945ffa10] sctp_datamsg_put at ffffffffa039c543 [sctp]
#8 [ffff8805945ffa60] sctp_datamsg_free at ffffffffa039c9dd [sctp]
#9 [ffff8805945ffa80] sctp_sendmsg at ffffffffa03a9440 [sctp]
#10 [ffff8805945ffb70] inet_sendmsg at ffffffff814ef0ba
#11 [ffff8805945ffbb0] sock_sendmsg at ffffffff8146b4c7
#12 [ffff8805945ffd60] __sys_sendmsg at ffffffff8146b976
#13 [ffff8805945fff10] sys_sendmsg at ffffffff8146bb99
In this case we have the chunk with 0 refcounter:
struct sctp_chunk {
list = {
next = 0xffff8809a84746d8,
prev = 0xffff880dbb1b0e80
},
refcnt = {
counter = 0
},
transmitted_list = {
next = 0xffff880dbb1b0d98,
prev = 0xffff880dbb1b0d98
},
frag_list = {
next = 0xffff880dbb1b0da8,
prev = 0xffff880dbb1b0da8
},
skb = 0xffff880dbb1a4700,
param_hdr = {
v = 0x0,
p = 0x0,
life = 0x0,
dns = 0x0,
cookie = 0x0,
....
Previous chunk in the list has the refcounter set to 2:
struct sctp_chunk {
list = {
next = 0xffff880dbb1b0d80,
prev = 0xffff880c657160c0
},
refcnt = {
counter = 2
},
transmitted_list = {
next = 0xffff880dbb1b0e98,
prev = 0xffff880dbb1b0e98
},
frag_list = {
next = 0xffff8808b025c300,
prev = 0xffff8808b025c300
},
skb = 0xffff880dbb1a4840,
param_hdr = {
v = 0x0,
p = 0x0,
life = 0x0,
dns = 0x0,
cookie = 0x0,
...
^ permalink raw reply
* Re: [PATCH 07/18] [media] uvcvideo: prevent bounds-check bypass via speculative execution
From: Dan Williams @ 2018-01-06 17:41 UTC (permalink / raw)
To: Greg KH
Cc: Linux Kernel Mailing List, linux-arch, Alan Cox, Peter Zijlstra,
Netdev, Laurent Pinchart, Thomas Gleixner, Mauro Carvalho Chehab,
Linus Torvalds, Elena Reshetova, Linux-media@vger.kernel.org, dsj
In-Reply-To: <20180106094026.GA11525@kroah.com>
On Sat, Jan 6, 2018 at 1:40 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Jan 06, 2018 at 10:09:07AM +0100, Greg KH wrote:
>> On Fri, Jan 05, 2018 at 05:10:32PM -0800, Dan Williams wrote:
>> > Static analysis reports that 'index' may be a user controlled value that
>> > is used as a data dependency to read 'pin' from the
>> > 'selector->baSourceID' array. In order to avoid potential leaks of
>> > kernel memory values, block speculative execution of the instruction
>> > stream that could issue reads based on an invalid value of 'pin'.
>> >
>> > Based on an original patch by Elena Reshetova.
>> >
>> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> > Cc: linux-media@vger.kernel.org
>> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
>> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> > ---
>> > drivers/media/usb/uvc/uvc_v4l2.c | 7 +++++--
>> > 1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
>> > index 3e7e283a44a8..7442626dc20e 100644
>> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> > @@ -22,6 +22,7 @@
>> > #include <linux/mm.h>
>> > #include <linux/wait.h>
>> > #include <linux/atomic.h>
>> > +#include <linux/compiler.h>
>> >
>> > #include <media/v4l2-common.h>
>> > #include <media/v4l2-ctrls.h>
>> > @@ -810,6 +811,7 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
>> > struct uvc_entity *iterm = NULL;
>> > u32 index = input->index;
>> > int pin = 0;
>> > + __u8 *elem;
>> >
>> > if (selector == NULL ||
>> > (chain->dev->quirks & UVC_QUIRK_IGNORE_SELECTOR_UNIT)) {
>> > @@ -820,8 +822,9 @@ static int uvc_ioctl_enum_input(struct file *file, void *fh,
>> > break;
>> > }
>> > pin = iterm->id;
>> > - } else if (index < selector->bNrInPins) {
>> > - pin = selector->baSourceID[index];
>> > + } else if ((elem = nospec_array_ptr(selector->baSourceID, index,
>> > + selector->bNrInPins))) {
>> > + pin = *elem;
>>
>> I dug through this before, and I couldn't find where index came from
>> userspace, I think seeing the coverity rule would be nice.
>
> Ok, I take it back, this looks correct. Ugh, the v4l ioctl api is
> crazy complex (rightfully so), it's amazing that coverity could navigate
> that whole thing :)
>
> While I'm all for fixing this type of thing, I feel like we need to do
> something "else" for this as playing whack-a-mole for this pattern is
> going to be a never-ending battle for all drivers for forever. Either
> we need some way to mark this data path to make it easy for tools like
> sparse to flag easily, or we need to catch the issue in the driver
> subsystems, which unfortunatly, would harm the drivers that don't have
> this type of issue (like here.)
>
> I'm guessing that other operating systems, which don't have the luxury
> of auditing all of their drivers are going for the "big hammer in the
> subsystem" type of fix, right?
>
> I don't have a good answer for this, but if there was some better way to
> rewrite these types of patterns to just prevent the need for the
> nospec_array_ptr() type thing, that might be the best overall for
> everyone. Much like ebpf did with their changes. That way a simple
> coccinelle rule would be able to catch the pattern and rewrite it.
>
> Or am I just dreaming?
At least on the coccinelle front you're dreaming. Julia already took a
look and said:
"I don't think Coccinelle would be good for doing this (ie
implementing taint analysis) because the dataflow is too complicated."
Perhaps the Coverity instance Dave mentioned at Ksummit 2012 has a
role to play here?
^ permalink raw reply
* Re: [patch net-next v6 00/11] net: sched: allow qdiscs to share filter block instances
From: David Ahern @ 2018-01-06 17:41 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, jhs, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, saeedm, matanb, leonro,
idosch, jakub.kicinski, simon.horman, pieter.jansenvanvuuren,
john.hurley, alexander.h.duyck, ogerlitz, john.fastabend, daniel
In-Reply-To: <20180106080728.GA2099@nanopsycho>
On 1/6/18 1:07 AM, Jiri Pirko wrote:
> Sat, Jan 06, 2018 at 04:57:21AM CET, dsahern@gmail.com wrote:
>> On 1/5/18 4:09 PM, Jiri Pirko wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> Currently the filters added to qdiscs are independent. So for example if you
>>> have 2 netdevices and you create ingress qdisc on both and you want to add
>>> identical filter rules both, you need to add them twice. This patchset
>>> makes this easier and mainly saves resources allowing to share all filters
>>> within a qdisc - I call it a "filter block". Also this helps to save
>>> resources when we do offload to hw for example to expensive TCAM.
>>>
>>> So back to the example. First, we create 2 qdiscs. Both will share
>>> block number 22. "22" is just an identification. If we don't pass any
>>> block number, a new one will be generated by kernel:
>>>
>>> $ tc qdisc add dev ens7 ingress block 22
>>> ^^^^^^^^
>>> $ tc qdisc add dev ens8 ingress block 22
>>> ^^^^^^^^
>>>
>>> Now if we list the qdiscs, we will see the block index in the output:
>>>
>>> $ tc qdisc
>>> qdisc ingress ffff: dev ens7 parent ffff:fff1 block 22
>>> qdisc ingress ffff: dev ens8 parent ffff:fff1 block 22
>>>
>>>
>>> To make is more visual, the situation looks like this:
>>>
>>> ens7 ingress qdisc ens7 ingress qdisc
>>> | |
>>> | |
>>> +----------> block 22 <----------+
>>>
>>> Unlimited number of qdiscs may share the same block.
>>>
>>> Now we can add filter using the block index:
>>>
>>> $ tc filter add block 22 protocol ip pref 25 flower dst_ip 192.168.0.0/16 action drop
>>>
>>>
>>> Note we cannot use the qdisc for filter manipulations for shared blocks:
>>>
>>> $ tc filter add dev ens8 ingress protocol ip pref 1 flower dst_ip 192.168.100.2 action drop
>>> Error: Cannot work with shared block, please use block index.
>>>
>>>
>>> We will see the same output if we list filters for ingress qdisc of
>>> ens7 and ens8, also for the block 22:
>>>
>>> $ tc filter show block 22
>>> filter block 22 protocol ip pref 25 flower chain 0
>>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>>> ...
>>>
>>> $ tc filter show dev ens7 ingress
>>> filter block 22 protocol ip pref 25 flower chain 0
>>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>>> ...
>>>
>>> $ tc filter show dev ens8 ingress
>>> filter block 22 protocol ip pref 25 flower chain 0
>>> filter block 22 protocol ip pref 25 flower chain 0 handle 0x1
>>> ...
>>
>> I like the API and output shown here, but I am not getting that with the
>> patches.
>>
>> In this example, I am using 42 for the block id:
>>
>> $ tc qdisc show dev eth2
>> qdisc mq 0: root
>> qdisc pfifo_fast 0: parent :2 bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1
>> 1 1 1
>> qdisc pfifo_fast 0: parent :1 bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1
>> 1 1 1
>> qdisc ingress ffff: parent ffff:fff1 block 42
>>
>> It allows me to add a filter using the device:
>> $ tc filter add dev eth2 ingress protocol ip pref 1 flower dst_ip
>> 192.168.101.2 action drop
>> $ echo $?
>> 0
>
> Yes, because the block is not shared yet. You have it only for one
> qdisc. As long as you have that, the "filter add dev" api still works.
> It stops working when you add another qdisc to that block.
Interesting.
Once I add the block to another qdisc I do get an error:
$ tc filter add dev eth2 ingress protocol ip pref 1 flower dst_ip
192.168.100.2 action drop
Error: Cannot work with shared block, please use block index.
Can you change that to something like: "This filter block is shared.
Please use the block index to make changes."
>
>
>>
>> And it modifies the shared block:
>> $ tc filter show block 42
>> filter pref 1 flower chain 0
>> filter pref 1 flower chain 0 handle 0x1
>> eth_type ipv4
>> dst_ip 192.168.100.2
>> not_in_hw
>> action order 1: gact action drop
>> random type none pass val 0
>> index 2 ref 1 bind 1
>>
>> filter pref 1 flower chain 0 handle 0x2
>> eth_type ipv4
>> dst_ip 192.168.101.2
>> not_in_hw
>> action order 1: gact action drop
>> random type none pass val 0
>> index 3 ref 1 bind 1
>>
>> filter pref 25 flower chain 0
>> filter pref 25 flower chain 0 handle 0x1
>> eth_type ipv4
>> dst_ip 192.168.0.0/16
>> not_in_hw
>> action order 1: gact action drop
>> random type none pass val 0
>> index 1 ref 1 bind 1
>>
>> Notice the header does not give the 'filter block N protocol' part. I
>> don't get that using the device either (tc filter show dev eth2 ingress).
>
> That is correct. Check the print_filter function in tc/tc_filter.c. It
> works with "filter_ifindex" and with my patch with "filter_block_index".
> That means that if the value for the filter dumped actually differs from
> what you passed on the command line, it prints it.
>
> Once you actually share the block with another qdisc, you will see
> "block N"
>
>
>>
>> Something else I noticed is that I do not get an error message if I pass
>> an invalid block id:
>>
>> $ tc filter show block 22
>> $ echo $?
>> 0
>> $ tc qdisc show | grep block
>> qdisc ingress ffff: dev eth2 parent ffff:fff1 block 42
>
> Yeah, I will try to fix this. The thing is, this is not error by kernel
> but by the userspace. Kernel is perfectly ok with invalid device or
> block index, it just does not dump anything and I would leave it like
> that. I have to somehow check the validity of block_index in userspace.
> Not sure how now.
Ok. I saw a response about idr_alloc_ext.
Here's another one: adding a filter to an unknown block id:
$ tc filter add block 66 ingress protocol ip pref 1 flower dst_ip
192.168.100.2 action drop
RTNETLINK answers: Invalid argument
We have an error talking to the kernel
Can you add a proper extack message for that case?
Thanks,
^ permalink raw reply
* Re: [PATCH net-next] net: tracepoint: adding new tracepoint arguments in inet_sock_set_state
From: Song Liu @ 2018-01-06 17:46 UTC (permalink / raw)
To: Yafang Shao
Cc: David Miller, brendan.d.gregg@gmail.com,
marcelo.leitner@gmail.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CALOAHbD+xe4HjQZ+2uhf8Ne66mNLLmAf+J6mkbbGfvTfitJYQw@mail.gmail.com>
> On Jan 5, 2018, at 12:09 AM, Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Fri, Jan 5, 2018 at 3:21 PM, Song Liu <songliubraving@fb.com> wrote:
>>
>>> On Jan 4, 2018, at 10:42 PM, Yafang Shao <laoar.shao@gmail.com> wrote:
>>>
>>> sk->sk_protocol and sk->sk_family are exposed as tracepoint arguments.
>>> Then we can conveniently use these two arguments to do the filter.
>>>
>>> Suggested-by: Brendan Gregg <brendan.d.gregg@gmail.com>
>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>> ---
>>> include/trace/events/sock.h | 24 ++++++++++++++++++------
>>> net/ipv4/af_inet.c | 6 ++++--
>>> 2 files changed, 22 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/trace/events/sock.h b/include/trace/events/sock.h
>>> index 3537c5f..c7df70f 100644
>>> --- a/include/trace/events/sock.h
>>> +++ b/include/trace/events/sock.h
>>> @@ -11,7 +11,11 @@
>>> #include <linux/ipv6.h>
>>> #include <linux/tcp.h>
>>>
>>> -/* The protocol traced by sock_set_state */
>>> +#define family_names \
>>> + EM(AF_INET) \
>>> + EMe(AF_INET6)
>>> +
>>> +/* The protocol traced by inet_sock_set_state */
>>> #define inet_protocol_names \
>>> EM(IPPROTO_TCP) \
>>> EM(IPPROTO_DCCP) \
>>> @@ -37,6 +41,7 @@
>>> #define EM(a) TRACE_DEFINE_ENUM(a);
>>> #define EMe(a) TRACE_DEFINE_ENUM(a);
>>>
>>> +family_names
>>> inet_protocol_names
>>> tcp_state_names
>>>
>>> @@ -45,6 +50,9 @@
>>> #define EM(a) { a, #a },
>>> #define EMe(a) { a, #a }
>>>
>>> +#define show_family_name(val) \
>>> + __print_symbolic(val, family_names)
>>> +
>>> #define show_inet_protocol_name(val) \
>>> __print_symbolic(val, inet_protocol_names)
>>>
>>> @@ -108,9 +116,10 @@
>>>
>>> TRACE_EVENT(inet_sock_set_state,
>>>
>>> - TP_PROTO(const struct sock *sk, const int oldstate, const int newstate),
>>> + TP_PROTO(const struct sock *sk, const int family, const int protocol,
>>> + const int oldstate, const int newstate),
>>
>> Are there cases we need protocol and/or family that is different to
>> sk->sk_protocol/sk_family? If not, I think we don't need to change the
>> TP_PROTO.
>>
>> Thanks,
>> Song
>>
>
> As of now, there're two sk_family, which are AF_INET and AF_INET6, and
> three sk_protocol which are IPPROTO_TCP, IPPROTO_SCTP and
> IPPROTO_DCCP.
>
> Thanks
> Yafang
How about we do not change the TP_PROTO line? The patch will be like:
@@ -118,6 +127,7 @@
__field(int, newstate)
__field(__u16, sport)
__field(__u16, dport)
+ __field(__u16, family)
__field(__u8, protocol)
__array(__u8, saddr, 4)
__array(__u8, daddr, 4)
@@ -133,8 +143,9 @@
__entry->skaddr = sk;
__entry->oldstate = oldstate;
__entry->newstate = newstate;
+ __entry->family = sk->sk_family;
__entry->protocol = sk->sk_protocol;
__entry->sport = ntohs(inet->inet_sport);
__entry->dport = ntohs(inet->inet_dport);
@@ xxxxx
- TP_printk("protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s",
+ TP_printk("family=%s protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s",
+ show_family_name(__entry->family),
Thanks,
Song
>>>
>>> - TP_ARGS(sk, oldstate, newstate),
>>> + TP_ARGS(sk, family, protocol, oldstate, newstate),
>>>
>>> TP_STRUCT__entry(
>>> __field(const void *, skaddr)
>>> @@ -118,6 +127,7 @@
>>> __field(int, newstate)
>>> __field(__u16, sport)
>>> __field(__u16, dport)
>>> + __field(__u16, family)
>>> __field(__u8, protocol)
>>> __array(__u8, saddr, 4)
>>> __array(__u8, daddr, 4)
>>> @@ -133,8 +143,9 @@
>>> __entry->skaddr = sk;
>>> __entry->oldstate = oldstate;
>>> __entry->newstate = newstate;
>>> + __entry->family = family;
>>> + __entry->protocol = protocol;
>>>
>>> - __entry->protocol = sk->sk_protocol;
>>> __entry->sport = ntohs(inet->inet_sport);
>>> __entry->dport = ntohs(inet->inet_dport);
>>>
>>> @@ -145,7 +156,7 @@
>>> *p32 = inet->inet_daddr;
>>>
>>> #if IS_ENABLED(CONFIG_IPV6)
>>> - if (sk->sk_family == AF_INET6) {
>>> + if (family == AF_INET6) {
>>> pin6 = (struct in6_addr *)__entry->saddr_v6;
>>> *pin6 = sk->sk_v6_rcv_saddr;
>>> pin6 = (struct in6_addr *)__entry->daddr_v6;
>>> @@ -160,7 +171,8 @@
>>> }
>>> ),
>>>
>>> - TP_printk("protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s",
>>> + TP_printk("family=%s protocol=%s sport=%hu dport=%hu saddr=%pI4 daddr=%pI4 saddrv6=%pI6c daddrv6=%pI6c oldstate=%s newstate=%s",
>>> + show_family_name(__entry->family),
>>> show_inet_protocol_name(__entry->protocol),
>>> __entry->sport, __entry->dport,
>>> __entry->saddr, __entry->daddr,
>>> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
>>> index bab98a4..1d52796 100644
>>> --- a/net/ipv4/af_inet.c
>>> +++ b/net/ipv4/af_inet.c
>>> @@ -1223,14 +1223,16 @@ int inet_sk_rebuild_header(struct sock *sk)
>>>
>>> void inet_sk_set_state(struct sock *sk, int state)
>>> {
>>> - trace_inet_sock_set_state(sk, sk->sk_state, state);
>>> + trace_inet_sock_set_state(sk, sk->sk_family, sk->sk_protocol,
>>> + sk->sk_state, state);
>>> sk->sk_state = state;
>>> }
>>> EXPORT_SYMBOL(inet_sk_set_state);
>>>
>>> void inet_sk_state_store(struct sock *sk, int newstate)
>>> {
>>> - trace_inet_sock_set_state(sk, sk->sk_state, newstate);
>>> + trace_inet_sock_set_state(sk, sk->sk_family, sk->sk_protocol,
>>> + sk->sk_state, newstate);
>>> smp_store_release(&sk->sk_state, newstate);
>>> }
>>>
>>> --
>>> 1.8.3.1
^ permalink raw reply
* Re: [PATCH 06/18] x86, barrier: stop speculation for failed access_ok
From: Linus Torvalds @ 2018-01-06 17:56 UTC (permalink / raw)
To: Alan Cox
Cc: Dan Williams, Linux Kernel Mailing List, linux-arch, Andi Kleen,
Arnd Bergmann, Greg Kroah-Hartman, Peter Zijlstra,
Network Development, the arch/x86 maintainers, Ingo Molnar,
H. Peter Anvin, Thomas Gleixner
In-Reply-To: <20180106123242.77f4d860@alans-desktop>
On Sat, Jan 6, 2018 at 4:32 AM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
>
> Also for x86-64 if we are trusting that an AND with a constant won't get
> speculated into something else surely we can just and the address with ~(1
> << 63) before copying from/to user space ? The user will then just
> speculatively steal their own memory.
User accesses *may* go to the kernel too, with set_fs(KERNEL_DS).
We've been getting rid of those, but they still exist.
We historically perhaps have 'and'ed the address with
current->thread.addr_limit, but it's no longer a power-of-two mask, so
that doesn't work. We'd have to play tricks there, but it might work
to do something like
addr &= current->thread.addr_limit | 0xfff;
or similar.
But this is one area where the 'lfence' is probably not too bad. The
cost of the existing 'stac' instruction is much higher, and in fact
depending on how lfence works (and how stac affects the memory unit
pipeline, it might even _help_ to serialize the stac with the actual
address.
Linus
^ permalink raw reply
* Re: [patch net-next v6 00/11] net: sched: allow qdiscs to share filter block instances
From: Jamal Hadi Salim @ 2018-01-06 18:02 UTC (permalink / raw)
To: Jiri Pirko, David Ahern
Cc: netdev, davem, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, saeedm, matanb, leonro,
idosch, jakub.kicinski, simon.horman, pieter.jansenvanvuuren,
john.hurley, alexander.h.duyck, ogerlitz, john.fastabend, daniel
In-Reply-To: <20180106094825.GB2099@nanopsycho>
On 18-01-06 04:48 AM, Jiri Pirko wrote:
[..]
> Or, do you think it should work like:
>
> $ tc qdisc add dev ens8 ingress
> $ tc qdisc
> qdisc ingress ffff: dev ens8 parent ffff:fff1
>
>
> $ tc qdisc add dev ens7 ingress block 22
>
> $ tc qdisc
> qdisc ingress ffff: dev ens7 parent ffff:fff1 block 22
> qdisc ingress ffff: dev ens8 parent ffff:fff1
>
> And the only shareable block is the one which I spefify block index for?
> And for that, I have to always use block index handle for filter
> add/del/get?
>
This is more explicit and better IMO (assuming syntax for sharing is
as shown earlier - adding via ens7 is going to be rejected).
Another thing to consider:
ens8 to join in the sharing i.e something like:
tc qdisc change/replace dev ens8 ingress block 22
And this to replace the local block created earlier
(with any old filters destroyed in the process).
BTW: From your output, DavidA, i noticed something strange:
two flower filters with the same handle id 0x1 (different prios)
and also two filters with the same prio (but different handles).
I see one was added using :dev .." - how were the other 2 added?
Consequence of patch, maybe?
Note:
Expected behavior is two filters of same kind on the same chain
should be distinguished by priority. There are filters like u32
(which hide hash tables under the same priority) which may
allow the same prio for multiple handles - just dont see that
fit with flower, but maybe missing something.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH 13/18] ipv6: prevent bounds-check bypass via speculative execution
From: Dan Williams @ 2018-01-06 18:05 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Linux Kernel Mailing List, linux-arch, Hideaki YOSHIFUJI, Netdev,
Peter Zijlstra, Greg KH, Alexey Kuznetsov, Thomas Gleixner,
Linus Torvalds, David S. Miller, Elena Reshetova, Alan Cox
In-Reply-To: <20180106064852.187637ad@xeon-e3>
On Sat, Jan 6, 2018 at 6:48 AM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Fri, 05 Jan 2018 17:11:04 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
>
>> Static analysis reports that 'offset' may be a user controlled value
>> that is used as a data dependency reading from a raw6_frag_vec buffer.
>> In order to avoid potential leaks of kernel memory values, block
>> speculative execution of the instruction stream that could issue further
>> reads based on an invalid '*(rfv->c + offset)' value.
>>
>> Based on an original patch by Elena Reshetova.
>>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
>> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>> net/ipv6/raw.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
>> index 761a473a07c5..384e3d59d148 100644
>> --- a/net/ipv6/raw.c
>> +++ b/net/ipv6/raw.c
>> @@ -33,6 +33,7 @@
>> #include <linux/skbuff.h>
>> #include <linux/compat.h>
>> #include <linux/uaccess.h>
>> +#include <linux/compiler.h>
>> #include <asm/ioctls.h>
>>
>> #include <net/net_namespace.h>
>> @@ -725,17 +726,17 @@ static int raw6_getfrag(void *from, char *to, int offset, int len, int odd,
>> struct sk_buff *skb)
>> {
>> struct raw6_frag_vec *rfv = from;
>> + char *rfv_buf;
>>
>> - if (offset < rfv->hlen) {
>> + if ((rfv_buf = nospec_array_ptr(rfv->c, offset, rfv->hlen))) {
>> int copy = min(rfv->hlen - offset, len);
>
> Minor nit.
>
> Please don't do assignment in condition test here.
> Instead.
> rfv_buf = nospec_array_ptr(rfv->c, offset, rfv->hlen);
> if (rfv_buf) {
Yeah, sorry about that. This was a hold over from an earlier version
where nospec_array_ptr() did not include the necessary barrier and we
relied on a new if_nospec helper, but now that if_nospec is no longer
being proposed I can go back and clean this up.
^ permalink raw reply
* Re: [PATCH 14/18] ipv4: prevent bounds-check bypass via speculative execution
From: Dan Williams @ 2018-01-06 18:10 UTC (permalink / raw)
To: Greg KH
Cc: Alan Cox, Linux Kernel Mailing List, linux-arch,
Hideaki YOSHIFUJI, Netdev, Peter Zijlstra, Alexey Kuznetsov,
Thomas Gleixner, Linus Torvalds, David S. Miller, Elena Reshetova,
Alan Cox
In-Reply-To: <CAPcyv4jsbwaBy29-L3rV__9AiMuZk7aO-0CguH6RNLYKgWWoYw@mail.gmail.com>
On Sat, Jan 6, 2018 at 8:29 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Sat, Jan 6, 2018 at 7:14 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Sat, Jan 06, 2018 at 12:23:47PM +0000, Alan Cox wrote:
>>> On Sat, 6 Jan 2018 10:01:54 +0100
>>> Greg KH <gregkh@linuxfoundation.org> wrote:
>>>
>>> > On Fri, Jan 05, 2018 at 05:11:10PM -0800, Dan Williams wrote:
>>> > > Static analysis reports that 'offset' may be a user controlled value
>>> >
>>> > Can I see the rule that determined that? It does not feel like that is
>>> > correct, given the 3+ levels deep that this function gets this value
>>> > from...
>>>
>>> On a current x86 you can execute something upwards of 150 instructions in
>>> a speculation window.
>>
>> Yeah, I agree, it's deep :(
>>
>> But for this patch, I thought the prior review determined that it was
>> not a problem. Was that somehow proven incorrect?
>
> I kept it in the series to get a re-review with the wider netdev
> because I missed the discussion leading up to that 'drop the patch'
> decision. Sorry, I should have noted that in the changelog or cover
> letter.
Is there a microbenchmark that can stress this path. If the cost is
negligible why play games with it being "probably ok"? Unless someone
can say 100% 'offset' is always bounded to something safe by the time
we get here just use nospec_array_ptr().
^ permalink raw reply
* Re: [PATCH 06/18] x86, barrier: stop speculation for failed access_ok
From: Alexei Starovoitov @ 2018-01-06 18:13 UTC (permalink / raw)
To: Alan Cox
Cc: Linus Torvalds, Dan Williams, linux-kernel, linux-arch,
Andi Kleen, Arnd Bergmann, Greg Kroah-Hartman, Peter Zijlstra,
netdev, Ingo Molnar, H. Peter Anvin, Thomas Gleixner
In-Reply-To: <20180106123242.77f4d860@alans-desktop>
On Sat, Jan 06, 2018 at 12:32:42PM +0000, Alan Cox wrote:
> On Fri, 5 Jan 2018 18:52:07 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > On Fri, Jan 5, 2018 at 5:10 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > >
> > > When access_ok fails we should always stop speculating.
> > > Add the required barriers to the x86 access_ok macro.
> >
> > Honestly, this seems completely bogus.
>
> Also for x86-64 if we are trusting that an AND with a constant won't get
> speculated into something else surely we can just and the address with ~(1
> << 63) before copying from/to user space ? The user will then just
> speculatively steal their own memory.
+1
Any type of straight line code can address variant 1.
Like changing:
array[index]
into
array[index & mask]
works even when 'mask' is a variable.
To proceed with speculative load from array cpu has to speculatively
load 'mask' from memory and speculatively do '&' alu.
If attacker cannot influence 'mask' the speculative value of it
will bound 'index & mask' value to be within array limits.
I think "lets sprinkle lfence everywhere" approach is going to
cause serious performance degradation. Yet people pushing for lfence
didn't present any numbers.
Last time lfence was removed from the networking drivers via dma_rmb()
packet-per-second metric jumped 10-30%. lfence forces all outstanding loads
to complete. If any prior load is waiting on L3 or memory,
lfence will cause 100+ ns stall and overall kernel performance will tank.
If kernel adopts this "lfence everywhere" approach it will be
the end of the kernel as we know it. All high performance operations
will move into user space. Networking and IO will be first.
Since it will takes years to design new cpus and even longer
to upgrade all servers the industry will have no choice,
but to move as much logic as possible from the kernel.
kpti already made crossing user/kernel boundary slower, but
kernel itself is still fast. If kernel will have lfence everywhere
the kernel itself will be slow.
In that sense retpolining the kernel is not as horrible as it sounds,
since both user space and kernel has to be retpolined.
^ permalink raw reply
* Re: [PATCH 1/2] SolutionEngine771x: fix Ether platform data
From: Sergei Shtylyov @ 2018-01-06 18:13 UTC (permalink / raw)
To: Yoshinori Sato, Rich Felker, linux-sh; +Cc: netdev
In-Reply-To: <20180103201133.743597164@cogentembedded.com>
Hello!
On 01/03/2018 11:08 PM, Sergei Shtylyov wrote:
> The 'sh_eth' driver's probe() method would fail on the SolutionEngine7710
> board and crash on SolutionEngine7712 board as the platform code is
> hopelessly behind the driver's platform data -- it passes the PHY address
> instead of 'struct sh_eth_plat_data *'; pass the latter to the driver in
> order to fix the bug...
>
> Fixes: 71557a37adb5 ("[netdrvr] sh_eth: Add SH7619 support")
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
[...]
Now that I was finally able to build an SH kernel, I realized that I
forgot to add #include <linux/sh_eth.h>... :-/
MBR, Sergei
^ permalink raw reply
* Re: [patch net-next v6 00/11] net: sched: allow qdiscs to share filter block instances
From: Jamal Hadi Salim @ 2018-01-06 18:16 UTC (permalink / raw)
To: David Ahern, Jiri Pirko
Cc: netdev, davem, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, saeedm, matanb, leonro,
idosch, jakub.kicinski, simon.horman, pieter.jansenvanvuuren,
john.hurley, alexander.h.duyck, ogerlitz, john.fastabend, daniel
In-Reply-To: <1b3e06a5-b07f-6de0-db1a-8add5ae8f140@gmail.com>
On 18-01-06 12:41 PM, David Ahern wrote:
> On 1/6/18 1:07 AM, Jiri Pirko wrote:
>> Sat, Jan 06, 2018 at 04:57:21AM CET, dsahern@gmail.com wrote:
>>> On 1/5/18 4:09 PM, Jiri Pirko wrote:
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>
>>> $ tc filter show block 22
>>> $ echo $?
>>> 0
>>> $ tc qdisc show | grep block
>>> qdisc ingress ffff: dev eth2 parent ffff:fff1 block 42
>>
>> Yeah, I will try to fix this. The thing is, this is not error by kernel
>> but by the userspace. Kernel is perfectly ok with invalid device or
>> block index, it just does not dump anything and I would leave it like
>> that. I have to somehow check the validity of block_index in userspace.
>> Not sure how now.
>
> Ok. I saw a response about idr_alloc_ext.
>
> Here's another one: adding a filter to an unknown block id:
>
> $ tc filter add block 66 ingress protocol ip pref 1 flower dst_ip
BTW, above syntax looks redundant because of the ingress parent
specification. It is possible iproute2/tc is just ignoring it right now
and the parent is not being used.
i.e Once the block is bound to ingress (aka parent ffff:) via:
qdisc ingress ffff: dev eth2 parent ffff:fff1 block 42
then
it doesnt make sense to specify a parent again because
it should be possible to bind that block to many parent locations
eg clsact ingress of dev x and clsact egress of dev y.
what am i missing?
cheers,
jamal
^ permalink raw reply
* Re: [PATCH 06/18] x86, barrier: stop speculation for failed access_ok
From: Dan Williams @ 2018-01-06 18:29 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alan Cox, Linus Torvalds, Linux Kernel Mailing List, linux-arch,
Andi Kleen, Arnd Bergmann, Greg Kroah-Hartman, Peter Zijlstra,
Netdev, Ingo Molnar, H. Peter Anvin, Thomas Gleixner
In-Reply-To: <20180106181331.mmrqwwbu2jcjj2si@ast-mbp>
On Sat, Jan 6, 2018 at 10:13 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Sat, Jan 06, 2018 at 12:32:42PM +0000, Alan Cox wrote:
>> On Fri, 5 Jan 2018 18:52:07 -0800
>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>
>> > On Fri, Jan 5, 2018 at 5:10 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> > > From: Andi Kleen <ak@linux.intel.com>
>> > >
>> > > When access_ok fails we should always stop speculating.
>> > > Add the required barriers to the x86 access_ok macro.
>> >
>> > Honestly, this seems completely bogus.
>>
>> Also for x86-64 if we are trusting that an AND with a constant won't get
>> speculated into something else surely we can just and the address with ~(1
>> << 63) before copying from/to user space ? The user will then just
>> speculatively steal their own memory.
>
> +1
>
> Any type of straight line code can address variant 1.
> Like changing:
> array[index]
> into
> array[index & mask]
> works even when 'mask' is a variable.
> To proceed with speculative load from array cpu has to speculatively
> load 'mask' from memory and speculatively do '&' alu.
> If attacker cannot influence 'mask' the speculative value of it
> will bound 'index & mask' value to be within array limits.
>
> I think "lets sprinkle lfence everywhere" approach is going to
> cause serious performance degradation. Yet people pushing for lfence
> didn't present any numbers.
> Last time lfence was removed from the networking drivers via dma_rmb()
> packet-per-second metric jumped 10-30%. lfence forces all outstanding loads
> to complete. If any prior load is waiting on L3 or memory,
> lfence will cause 100+ ns stall and overall kernel performance will tank.
You are conflating dma_rmb() with the limited cases where
nospec_array_ptr() is used. I need help determining what the
performance impact of those limited places are.
> If kernel adopts this "lfence everywhere" approach it will be
> the end of the kernel as we know it. All high performance operations
> will move into user space. Networking and IO will be first.
> Since it will takes years to design new cpus and even longer
> to upgrade all servers the industry will have no choice,
> but to move as much logic as possible from the kernel.
>
> kpti already made crossing user/kernel boundary slower, but
> kernel itself is still fast. If kernel will have lfence everywhere
> the kernel itself will be slow.
>
> In that sense retpolining the kernel is not as horrible as it sounds,
> since both user space and kernel has to be retpolined.
retpoline is variant-2, this patch series is about variant-1.
^ permalink raw reply
* Re: [patch net-next v6 00/11] net: sched: allow qdiscs to share filter block instances
From: Jamal Hadi Salim @ 2018-01-06 18:31 UTC (permalink / raw)
To: Jiri Pirko, David Ahern
Cc: netdev, davem, xiyou.wangcong, mlxsw, andrew, vivien.didelot,
f.fainelli, michael.chan, ganeshgr, saeedm, matanb, leonro,
idosch, jakub.kicinski, simon.horman, pieter.jansenvanvuuren,
john.hurley, alexander.h.duyck, ogerlitz, john.fastabend, daniel
In-Reply-To: <c658f390-3996-d1e2-7e43-0cec6eb03c67@mojatatu.com>
On 18-01-06 01:02 PM, Jamal Hadi Salim wrote:
> On 18-01-06 04:48 AM, Jiri Pirko wrote:
>
> BTW: From your output, DavidA, i noticed something strange:
> two flower filters with the same handle id 0x1 (different prios)
At least on the kernel i am using this is the exhibited default
behavior. I can also create filters with different priorities
and with different handle ids.
> and also two filters with the same prio (but different handles).
This does not seem possible (as i was expecting) - so could be
the result of the block code...
cheers,
jamal
^ permalink raw reply
* Re: [PATCH 06/18] x86, barrier: stop speculation for failed access_ok
From: Alan Cox @ 2018-01-06 18:38 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Linus Torvalds, Dan Williams, linux-kernel, linux-arch,
Andi Kleen, Arnd Bergmann, Greg Kroah-Hartman, Peter Zijlstra,
netdev, Ingo Molnar, H. Peter Anvin, Thomas Gleixner
In-Reply-To: <20180106181331.mmrqwwbu2jcjj2si@ast-mbp>
On Sat, 6 Jan 2018 10:13:33 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> On Sat, Jan 06, 2018 at 12:32:42PM +0000, Alan Cox wrote:
> > On Fri, 5 Jan 2018 18:52:07 -0800
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> > > On Fri, Jan 5, 2018 at 5:10 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> > > > From: Andi Kleen <ak@linux.intel.com>
> > > >
> > > > When access_ok fails we should always stop speculating.
> > > > Add the required barriers to the x86 access_ok macro.
> > >
> > > Honestly, this seems completely bogus.
> >
> > Also for x86-64 if we are trusting that an AND with a constant won't get
> > speculated into something else surely we can just and the address with ~(1
> > << 63) before copying from/to user space ? The user will then just
> > speculatively steal their own memory.
>
> +1
>
> Any type of straight line code can address variant 1.
> Like changing:
> array[index]
> into
> array[index & mask]
> works even when 'mask' is a variable.
That statement is unfortunately not one that we currently believe is
true for all architectures, platforms and implementations. It may be true
for some architectures but processors can speculate on more than just
execution paths. For some architecutres it may be the right way to
implement Linus array_* methods.
> I think "lets sprinkle lfence everywhere" approach is going to
> cause serious performance degradation. Yet people pushing for lfence
> didn't present any numbers.
Normally people who propose security fixes don't have to argue about the
fact they added 30 clocks to avoid your box being 0wned.
If you look at the patches very very few are in remotely hot paths, which
is good news. The ones in hot paths definitely need careful scrutiny but
the priority ought to be fixing and then optimizing unless its obvious
how to tackle it.
Alan
^ permalink raw reply
* Re: [PATCH 06/18] x86, barrier: stop speculation for failed access_ok
From: Alexei Starovoitov @ 2018-01-06 18:39 UTC (permalink / raw)
To: Dan Williams
Cc: Alan Cox, Linus Torvalds, Linux Kernel Mailing List, linux-arch,
Andi Kleen, Arnd Bergmann, Greg Kroah-Hartman, Peter Zijlstra,
Netdev, Ingo Molnar, H. Peter Anvin, Thomas Gleixner
In-Reply-To: <CAPcyv4jqKmnkL1CfHVccHvocmSD4PamqOy4bPsO1789D+107FQ@mail.gmail.com>
On Sat, Jan 06, 2018 at 10:29:49AM -0800, Dan Williams wrote:
> On Sat, Jan 6, 2018 at 10:13 AM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Sat, Jan 06, 2018 at 12:32:42PM +0000, Alan Cox wrote:
> >> On Fri, 5 Jan 2018 18:52:07 -0800
> >> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >>
> >> > On Fri, Jan 5, 2018 at 5:10 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> >> > > From: Andi Kleen <ak@linux.intel.com>
> >> > >
> >> > > When access_ok fails we should always stop speculating.
> >> > > Add the required barriers to the x86 access_ok macro.
> >> >
> >> > Honestly, this seems completely bogus.
> >>
> >> Also for x86-64 if we are trusting that an AND with a constant won't get
> >> speculated into something else surely we can just and the address with ~(1
> >> << 63) before copying from/to user space ? The user will then just
> >> speculatively steal their own memory.
> >
> > +1
> >
> > Any type of straight line code can address variant 1.
> > Like changing:
> > array[index]
> > into
> > array[index & mask]
> > works even when 'mask' is a variable.
> > To proceed with speculative load from array cpu has to speculatively
> > load 'mask' from memory and speculatively do '&' alu.
> > If attacker cannot influence 'mask' the speculative value of it
> > will bound 'index & mask' value to be within array limits.
> >
> > I think "lets sprinkle lfence everywhere" approach is going to
> > cause serious performance degradation. Yet people pushing for lfence
> > didn't present any numbers.
> > Last time lfence was removed from the networking drivers via dma_rmb()
> > packet-per-second metric jumped 10-30%. lfence forces all outstanding loads
> > to complete. If any prior load is waiting on L3 or memory,
> > lfence will cause 100+ ns stall and overall kernel performance will tank.
>
> You are conflating dma_rmb() with the limited cases where
> nospec_array_ptr() is used. I need help determining what the
> performance impact of those limited places are.
really? fdtable, access_ok, net/ipv[46] is not critical path?
> > If kernel adopts this "lfence everywhere" approach it will be
> > the end of the kernel as we know it. All high performance operations
> > will move into user space. Networking and IO will be first.
> > Since it will takes years to design new cpus and even longer
> > to upgrade all servers the industry will have no choice,
> > but to move as much logic as possible from the kernel.
> >
> > kpti already made crossing user/kernel boundary slower, but
> > kernel itself is still fast. If kernel will have lfence everywhere
> > the kernel itself will be slow.
> >
> > In that sense retpolining the kernel is not as horrible as it sounds,
> > since both user space and kernel has to be retpolined.
>
> retpoline is variant-2, this patch series is about variant-1.
that's exactly the point. Don't slow down the kernel with lfences
to solve variant 1. retpoline for 2 is ok from long term kernel
viability perspective.
^ permalink raw reply
* Re: [PATCH 06/18] x86, barrier: stop speculation for failed access_ok
From: Alexei Starovoitov @ 2018-01-06 18:51 UTC (permalink / raw)
To: Alan Cox
Cc: Linus Torvalds, Dan Williams, linux-kernel, linux-arch,
Andi Kleen, Arnd Bergmann, Greg Kroah-Hartman, Peter Zijlstra,
netdev, Ingo Molnar, H. Peter Anvin, Thomas Gleixner
In-Reply-To: <20180106183859.1ad9ae37@alans-desktop>
On Sat, Jan 06, 2018 at 06:38:59PM +0000, Alan Cox wrote:
> On Sat, 6 Jan 2018 10:13:33 -0800
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > On Sat, Jan 06, 2018 at 12:32:42PM +0000, Alan Cox wrote:
> > > On Fri, 5 Jan 2018 18:52:07 -0800
> > > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > >
> > > > On Fri, Jan 5, 2018 at 5:10 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> > > > > From: Andi Kleen <ak@linux.intel.com>
> > > > >
> > > > > When access_ok fails we should always stop speculating.
> > > > > Add the required barriers to the x86 access_ok macro.
> > > >
> > > > Honestly, this seems completely bogus.
> > >
> > > Also for x86-64 if we are trusting that an AND with a constant won't get
> > > speculated into something else surely we can just and the address with ~(1
> > > << 63) before copying from/to user space ? The user will then just
> > > speculatively steal their own memory.
> >
> > +1
> >
> > Any type of straight line code can address variant 1.
> > Like changing:
> > array[index]
> > into
> > array[index & mask]
> > works even when 'mask' is a variable.
>
> That statement is unfortunately not one that we currently believe is
> true for all architectures, platforms and implementations. It may be true
> for some architectures but processors can speculate on more than just
> execution paths. For some architecutres it may be the right way to
> implement Linus array_* methods.
please name one where cpu will ignore above '&' and speculate
load from array based on 'index' only.
cpus execute what they see. speculative execution does the same
except results are not committed to visible registers and stay
in renanmed/shadow set. There is no 'undo' of the speculative execution.
The whole issue is that cache and branch predictor don't have
a shadow unlike registers.
> > I think "lets sprinkle lfence everywhere" approach is going to
> > cause serious performance degradation. Yet people pushing for lfence
> > didn't present any numbers.
>
> Normally people who propose security fixes don't have to argue about the
> fact they added 30 clocks to avoid your box being 0wned.
>
> If you look at the patches very very few are in remotely hot paths, which
> is good news. The ones in hot paths definitely need careful scrutiny but
> the priority ought to be fixing and then optimizing unless its obvious
> how to tackle it.
fdtable, access_ok and networking are clearly hot, no?
^ permalink raw reply
* [PATCH 0/2] Ether fixes for the SolutionEngine771x boards
From: Sergei Shtylyov @ 2018-01-06 18:53 UTC (permalink / raw)
To: Yoshinori Sato, Rich Felker, linux-sh; +Cc: netdev, Sergei Shtylyov
Hello!
Here's the series of 2 patches against Linus' repo. This series should
(hoplefully) fix the Ether support on the SolutionEngine771x boards...
[1/2] SolutionEngine771x: fix Ether platform data
[2/2] SolutionEngine771x: add Ether TSU resource
MBR, Sergei
^ permalink raw reply
* [PATCH 1/2] SolutionEngine771x: fix Ether platform data
From: Sergei Shtylyov @ 2018-01-06 18:53 UTC (permalink / raw)
To: Yoshinori Sato, Rich Felker, linux-sh; +Cc: netdev, Sergei Shtylyov
[-- Attachment #1: SolutionEngine771x-fix-Ether-platform-data-v2.patch --]
[-- Type: text/plain, Size: 1914 bytes --]
The 'sh_eth' driver's probe() method would fail on the SolutionEngine7710
board and crash on SolutionEngine7712 board as the platform code is
hopelessly behind the driver's platform data -- it passes the PHY address
instead of 'struct sh_eth_plat_data *'; pass the latter to the driver in
order to fix the bug...
Fixes: 71557a37adb5 ("[netdrvr] sh_eth: Add SH7619 support")
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
Changes in version 2:
- added the necessary #include.
arch/sh/boards/mach-se/770x/setup.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
Index: linux/arch/sh/boards/mach-se/770x/setup.c
===================================================================
--- linux.orig/arch/sh/boards/mach-se/770x/setup.c
+++ linux/arch/sh/boards/mach-se/770x/setup.c
@@ -9,6 +9,7 @@
*/
#include <linux/init.h>
#include <linux/platform_device.h>
+#include <linux/sh_eth.h>
#include <mach-se/mach/se.h>
#include <mach-se/mach/mrshpc.h>
#include <asm/machvec.h>
@@ -115,6 +116,11 @@ static struct platform_device heartbeat_
#if defined(CONFIG_CPU_SUBTYPE_SH7710) ||\
defined(CONFIG_CPU_SUBTYPE_SH7712)
/* SH771X Ethernet driver */
+static struct sh_eth_plat_data sh_eth_plat = {
+ .phy = PHY_ID,
+ .phy_interface = PHY_INTERFACE_MODE_MII,
+};
+
static struct resource sh_eth0_resources[] = {
[0] = {
.start = SH_ETH0_BASE,
@@ -132,7 +138,7 @@ static struct platform_device sh_eth0_de
.name = "sh771x-ether",
.id = 0,
.dev = {
- .platform_data = PHY_ID,
+ .platform_data = &sh_eth_plat,
},
.num_resources = ARRAY_SIZE(sh_eth0_resources),
.resource = sh_eth0_resources,
@@ -155,7 +161,7 @@ static struct platform_device sh_eth1_de
.name = "sh771x-ether",
.id = 1,
.dev = {
- .platform_data = PHY_ID,
+ .platform_data = &sh_eth_plat,
},
.num_resources = ARRAY_SIZE(sh_eth1_resources),
.resource = sh_eth1_resources,
^ permalink raw reply
* [PATCH 2/2] SolutionEngine771x: add Ether TSU resource
From: Sergei Shtylyov @ 2018-01-06 18:53 UTC (permalink / raw)
To: Yoshinori Sato, Rich Felker, linux-sh; +Cc: netdev, Sergei Shtylyov
[-- Attachment #1: SolutionEngine771x-add-Ether-TSU-resource-v2.patch --]
[-- Type: text/plain, Size: 2212 bytes --]
After the Ether platform data is fixed, the driver probe() method would
still fail since the 'struct sh_eth_cpu_data' corresponding to SH771x
indicates the presence of TSU but the memory resource for it is absent.
Add the missing TSU resource to both Ether devices and fix the harmless
off-by-one error in the main memory resources, while at it...
Fixes: 4986b996882d ("net: sh_eth: remove the SH_TSU_ADDR")
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
Changes in version 2:
- corrected the TSU resource size.
arch/sh/boards/mach-se/770x/setup.c | 14 ++++++++++++--
arch/sh/include/mach-se/mach/se.h | 1 +
2 files changed, 13 insertions(+), 2 deletions(-)
Index: linux/arch/sh/boards/mach-se/770x/setup.c
===================================================================
--- linux.orig/arch/sh/boards/mach-se/770x/setup.c
+++ linux/arch/sh/boards/mach-se/770x/setup.c
@@ -124,10 +124,15 @@ static struct sh_eth_plat_data sh_eth_pl
static struct resource sh_eth0_resources[] = {
[0] = {
.start = SH_ETH0_BASE,
- .end = SH_ETH0_BASE + 0x1B8,
+ .end = SH_ETH0_BASE + 0x1B8 - 1,
.flags = IORESOURCE_MEM,
},
[1] = {
+ .start = SH_TSU_BASE,
+ .end = SH_TSU_BASE + 0x200 - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ [2] = {
.start = SH_ETH0_IRQ,
.end = SH_ETH0_IRQ,
.flags = IORESOURCE_IRQ,
@@ -147,10 +152,15 @@ static struct platform_device sh_eth0_de
static struct resource sh_eth1_resources[] = {
[0] = {
.start = SH_ETH1_BASE,
- .end = SH_ETH1_BASE + 0x1B8,
+ .end = SH_ETH1_BASE + 0x1B8 - 1,
.flags = IORESOURCE_MEM,
},
[1] = {
+ .start = SH_TSU_BASE,
+ .end = SH_TSU_BASE + 0x200 - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ [2] = {
.start = SH_ETH1_IRQ,
.end = SH_ETH1_IRQ,
.flags = IORESOURCE_IRQ,
Index: linux/arch/sh/include/mach-se/mach/se.h
===================================================================
--- linux.orig/arch/sh/include/mach-se/mach/se.h
+++ linux/arch/sh/include/mach-se/mach/se.h
@@ -100,6 +100,7 @@
/* Base address */
#define SH_ETH0_BASE 0xA7000000
#define SH_ETH1_BASE 0xA7000400
+#define SH_TSU_BASE 0xA7000800
/* PHY ID */
#if defined(CONFIG_CPU_SUBTYPE_SH7710)
# define PHY_ID 0x00
^ 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