* Re: unregister_netdevice: waiting for DEV to become free (2)
From: Julian Anastasov @ 2018-08-22 4:11 UTC (permalink / raw)
To: Cong Wang
Cc: syzbot+30209ea299c09d8785c9, ddstreet, Dmitry Vyukov, LKML,
Linux Kernel Network Developers, syzkaller-bugs
In-Reply-To: <CAM_iQpVM4wHkW4RKMuDj_Jjof3XbJmAsN0SdSRfneaf94CL0cw@mail.gmail.com>
Hello,
On Mon, 20 Aug 2018, Cong Wang wrote:
> For the one I look into, dst_cache doesn't matter, because the xmit
> path doesn't even use tunnel dst_cache at all, and it is ip6tnl0 FB
> device, unlike this one which is tun device.
Ops, of course, it is dev tun and not ip tun...
> For the one I look into, it seems some fib6_info is not released for
> some reason. It seems to be the one created by addrconf_prefix_route(),
> which is supposed to be released by fib6_clean_tree() I think, but it
> never happens.
May be, it is not direct reference to dev but one
that is moved to loopback, like from dst, route... The repro.c
creates permanent neighbours and addresses.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH v1 3/3] net: WireGuard secure network tunnel
From: Andrew Lunn @ 2018-08-22 0:23 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: David Miller, Stephen Hemminger, Netdev, Greg Kroah-Hartman
In-Reply-To: <CAHmME9qriuodvMo3FyfA0TEkRubxu7_Dbs=0aFrF6XOcZZtQfg@mail.gmail.com>
On Tue, Aug 21, 2018 at 04:59:52PM -0700, Jason A. Donenfeld wrote:
> On Tue, Aug 21, 2018 at 4:54 PM David Miller <davem@davemloft.net> wrote:
> >
> > From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> > Date: Tue, 21 Aug 2018 16:41:50 -0700
> >
> > > Is 100 in fact acceptable for new code? 120? 180? What's the
> > > generally accepted limit these days?
> >
> > Please keep it as close to 80 columns as possible.
> >
> > Line breaks are not ugly, please embrace them :)
>
> Okay! Will do. Thanks for the response.
Hi Jason
I find the coding style document,
Documentation/process/coding-style.rst makes good arguments about why
it is like this:
Now, some people will claim that having 8-character indentations
makes the code move too far to the right, and makes it hard to read
on a 80-character terminal screen. The answer to that is that if
you need more than 3 levels of indentation, you're screwed anyway,
and should fix your program.
In short, 8-char indents make things easier to read, and have the
added benefit of warning you when you're nesting your functions too
deep. Heed that warning.
If you were to decided on 100, you might then want to go more than 3
levels deep, rather than heed the warning and refactor the code into
lots of smaller functions.
There is also:
Functions should be short and sweet, and do just one thing. They
should fit on one or two screenfuls of text (the ISO/ANSI screen
size is 80x24, as we all know), and do one thing and do that well.
It is worth reading the document in full, just for the rationale.
Andrew
^ permalink raw reply
* Re: [bpf-next RFC 0/3] Introduce eBPF flow dissector
From: Petar Penkov @ 2018-08-22 0:19 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Petar Penkov, Networking, David S . Miller, Alexei Starovoitov,
Daniel Borkmann, simon.horman, Willem de Bruijn
In-Reply-To: <20180820205205.jj7e75pulwqkorpr@ast-mbp>
On Mon, Aug 20, 2018 at 1:52 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Aug 16, 2018 at 09:44:20AM -0700, Petar Penkov wrote:
>> From: Petar Penkov <ppenkov@google.com>
>>
>> This patch series hardens the RX stack by allowing flow dissection in BPF,
>> as previously discussed [1]. Because of the rigorous checks of the BPF
>> verifier, this provides significant security guarantees. In particular, the
>> BPF flow dissector cannot get inside of an infinite loop, as with
>> CVE-2013-4348, because BPF programs are guaranteed to terminate. It cannot
>> read outside of packet bounds, because all memory accesses are checked.
>> Also, with BPF the administrator can decide which protocols to support,
>> reducing potential attack surface. Rarely encountered protocols can be
>> excluded from dissection and the program can be updated without kernel
>> recompile or reboot if a bug is discovered.
>>
>> Patch 1 adds infrastructure to execute a BPF program in __skb_flow_dissect.
>> This includes a new BPF program and attach type.
>>
>> Patch 2 adds a flow dissector program in BPF. This parses most protocols in
>> __skb_flow_dissect in BPF for a subset of flow keys (basic, control, ports,
>> and address types).
>>
>> Patch 3 adds a selftest that attaches the BPF program to the flow dissector
>> and sends traffic with different levels of encapsulation.
>
> Overall I fully support the direction. Few things to consider:
>
>> This RFC patchset exposes a few design considerations:
>>
>> 1/ Because the flow dissector key definitions live in
>> include/linux/net/flow_dissector.h, they are not visible from userspace,
>> and the flow keys definitions need to be copied in the BPF program.
>
> I don't think copy-paste avoids the issue of uapi.
> Anything used by BPF program is uapi.
> The only exception is offsets of kernel internal structures
> passed into bpf_probe_read().
> So we have several options:
> 1. be honest and say 'struct flow_dissect_key*' is now uapi
> 2. wrap all of them into 'struct bpf_flow_dissect_key*' and do rewrites
> when/if 'struct flow_dissect_key*' changes
> 3. wait for BTF to solve it for tracing use case and for this one two.
> The idea is that kernel internal structs can be defined in bpf prog
> and since they will be described precisely in BTF that comes with the prog
> the kernel can validate that prog's BTF matches what kernel thinks it has.
> imo that's the most flexible, but BTF for all of vmlinux won't be ready
> tomorrow and looks like this patch set is ready to go, so I would go with 1 or 2.
I will pursue #2 then as both you and David are in support of it.
>
>> 2/ An alternative to adding a new hook would have been to attach flow
>> dissection programs at the XDP hook. Because this hook is executed before
>> GRO, it would have to execute on every MSS, which would be more
>> computationally expensive. Furthermore, the XDP hook is executed before an
>> SKB has been allocated and there is no clear way to move the dissected keys
>> into the SKB after it has been allocated. Eventually, perhaps a single pass
>> can implement both GRO and flow dissection -- but napi_gro_cb shows that a
>> lot more flow state would need to be parsed for this.
>
> global flow_dissect bpf hook semantics are problematic for testing.
> like patch 3 test affects the whole system including all containers.
> should the hook be per-netns or may be per netdevice?
>
Having the hook be per-netns would definitely be cleaner for testing.
I will look into
refactoring the hook from global to per-netns.
>> 3/ The BPF program cannot use direct packet access everywhere because it
>> uses an offset, initially supplied by the flow dissector. Because the
>> initial value of this non-constant offset comes from outside of the
>> program, the verifier does not know what its value is, and it cannot verify
>> that it is within packet bounds. Therefore, direct packet access programs
>> get rejected.
>
> this part doesn't seem to match the code.
> direct packet access is allowed and usable even for fragmented skbs.
> in such case only linear part of skb is in "direct access".
I am not sure I understand. What I meant was that I use bpf_skb_load_bytes
rather than direct packet access because the offset at which I read headers,
nhoff, depends on an initial value that cannot be statically verified - namely
what __skb_flow_dissect provides. Is there an alternative approach I should
be taking here, and/or am I misunderstanding direct access?
>
> Last bit, I'm curious about, how the 'demo' flow dissector program
> from patch 2 fairs vs in-kernel dissector when performance tested?
>
I used the test in patch 3 to compare the two implementations and the
difference seemed to be small, with the BPF flow dissector being slightly
slower.
Thank you so much for your feedback, Alexei and David!
^ permalink raw reply
* Re: [PATCH bpf] bpf, sockmap: fix sock_hash_alloc and reject zero-sized keys
From: Song Liu @ 2018-08-22 0:12 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Alexei Starovoitov, John Fastabend, Networking
In-Reply-To: <e444ac8bc5dce13f118d43f0277f5f0bccf4402d.1534888782.git.daniel@iogearbox.net>
On Tue, Aug 21, 2018 at 3:02 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> Currently, it is possible to create a sock hash map with key size
> of 0 and have the kernel return a fd back to user space. This is
> invalid for hash maps (and kernel also hasn't been tested for zero
> key size support in general at this point). Thus, reject such
> configuration.
>
> Fixes: 81110384441a ("bpf: sockmap, add hash map support")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Acked-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> kernel/bpf/sockmap.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
> index 98e621a..60ceb0e 100644
> --- a/kernel/bpf/sockmap.c
> +++ b/kernel/bpf/sockmap.c
> @@ -2140,7 +2140,9 @@ static struct bpf_map *sock_hash_alloc(union bpf_attr *attr)
> return ERR_PTR(-EPERM);
>
> /* check sanity of attributes */
> - if (attr->max_entries == 0 || attr->value_size != 4 ||
> + if (attr->max_entries == 0 ||
> + attr->key_size == 0 ||
> + attr->value_size != 4 ||
> attr->map_flags & ~SOCK_CREATE_FLAG_MASK)
> return ERR_PTR(-EINVAL);
>
> --
> 2.9.5
>
^ permalink raw reply
* Re: [PATCH v1 3/3] net: WireGuard secure network tunnel
From: Jason A. Donenfeld @ 2018-08-21 23:59 UTC (permalink / raw)
To: David Miller; +Cc: Stephen Hemminger, Netdev, Greg Kroah-Hartman
In-Reply-To: <20180821.165442.1494103888623363991.davem@davemloft.net>
On Tue, Aug 21, 2018 at 4:54 PM David Miller <davem@davemloft.net> wrote:
>
> From: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Date: Tue, 21 Aug 2018 16:41:50 -0700
>
> > Is 100 in fact acceptable for new code? 120? 180? What's the
> > generally accepted limit these days?
>
> Please keep it as close to 80 columns as possible.
>
> Line breaks are not ugly, please embrace them :)
Okay! Will do. Thanks for the response.
Jason
^ permalink raw reply
* Re: [PATCH v1 3/3] net: WireGuard secure network tunnel
From: David Miller @ 2018-08-21 23:54 UTC (permalink / raw)
To: Jason; +Cc: stephen, netdev, gregkh
In-Reply-To: <CAHmME9oKvhQhn-8tjpNfKFgNPLH9aZAPO=iEwi2M4w2YBa5JBQ@mail.gmail.com>
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Tue, 21 Aug 2018 16:41:50 -0700
> Is 100 in fact acceptable for new code? 120? 180? What's the
> generally accepted limit these days?
Please keep it as close to 80 columns as possible.
Line breaks are not ugly, please embrace them :)
^ permalink raw reply
* Re: [PATCH v1 3/3] net: WireGuard secure network tunnel
From: Jason A. Donenfeld @ 2018-08-21 23:41 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Netdev, David Miller, Greg Kroah-Hartman
In-Reply-To: <20180731132204.2bc30d15@xeon-e3>
On Tue, Jul 31, 2018 at 1:22 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 31 Jul 2018 21:11:02 +0200
> "Jason A. Donenfeld" <Jason@zx2c4.com> wrote:
>
> > +static int walk_by_peer(struct allowedips_node __rcu *top, u8 bits, struct allowedips_cursor *cursor, struct wireguard_peer *peer, int (*func)(void *ctx, const u8 *ip, u8 cidr, int family), void *ctx, struct mutex *lock)
> > +{
>
> Please break lines at something reasonable like 100 characters.
As I mentioned earlier, things will certainly be that way for v2, and
I'm in the process of that. I do care quite a bit about having "pretty
code", and so while I'm wrapping, I want to do it well and
consistently. Indeed 100 characters is far more reasonable than 80 --
things wind up much less annoying. And 120 is even easier. I don't
want to have to start renaming nice descriptive struct names and nice
descriptive struct member names just to avoid the line breaks in
function signatures, or something silly like that. In any case, before
I take out my word wrapping paint brush and make the prettiest
wrapping I can, I wanted to double check and confirm what the norm is
for the net tree. Is 100 in fact acceptable for new code? 120? 180?
What's the generally accepted limit these days?
(And, folks, please don't let this inquiry spiral out of control into
a bikeshed thread. I'm narrowly interested in knowing what the facts
are on what the norm is and what will be accepted here, rather than a
cacophony of random opinions.)
^ permalink raw reply
* Re: [PATCH] datapath.c: fix missing return value check of nla_nest_start()
From: David Miller @ 2018-08-21 23:38 UTC (permalink / raw)
To: pshelar; +Cc: jasonwood2031, netdev
In-Reply-To: <CAOrHB_DgqCAGcMnyDngmvwtC8fvX9epp6KW5+DV7kn9LbBWkyQ@mail.gmail.com>
From: Pravin Shelar <pshelar@ovn.org>
Date: Tue, 21 Aug 2018 15:38:28 -0700
> On Fri, Aug 17, 2018 at 1:15 AM Jiecheng Wu <jasonwood2031@gmail.com> wrote:
>>
>> Function queue_userspace_packet() defined in net/openvswitch/datapath.c calls nla_nest_start() to allocate memory for struct nlattr which is dereferenced immediately. As nla_nest_start() may return NULL on failure, this code piece may cause NULL pointer dereference bug.
>> ---
>> net/openvswitch/datapath.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
>> index 0f5ce77..ff4457d 100644
>> --- a/net/openvswitch/datapath.c
>> +++ b/net/openvswitch/datapath.c
>> @@ -460,6 +460,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>>
>> if (upcall_info->egress_tun_info) {
>> nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_EGRESS_TUN_KEY);
>> + if (!nla)
>> + return -EMSGSIZE;
> It is not possible, since user_skb is allocated to accommodate all
> netlink attributes.
Pravin, common practice is to always check nla_*() return values even if the
SKB is allocated with "enough space".
Those calculations can have bugs, and these checks are therefore helpful to
avoid crashes and memory corruption in such cases.
Thank you.
^ permalink raw reply
* Re: Experimental fix for MSI-X issue on r8169
From: David Miller @ 2018-08-21 23:32 UTC (permalink / raw)
To: hkallweit1; +Cc: jian-hong, steved424, gogen, netdev
In-Reply-To: <98df6b0a-44df-9c0d-ddf6-11b892ccd990@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Tue, 21 Aug 2018 23:19:04 +0200
> That's what I get on my system (RTL8168E-VL). In your case you'll come
> only till the first suspend.
>
> [ 3.743404] r8169 0000:03:00.0: MSI-X entry: context probe: fee01004 0 40ef 1
On probe, MSI-X is masked (ie. disabled) and is configured to use:
address: 0x00000000fee01004
data: 0x00000000000040ef
> [ 29.539250] r8169 0000:03:00.0: MSI-X entry: context suspend: fee02004 0 4028 0
At suspend time, MSI-X is unmasked (ie. enabled) and is configured to use:
address: 0x00000000fee01004
data: 0x0000000000004028
> [ 29.837457] r8169 0000:03:00.0: MSI-X entry: context resume: fee01004 0 402b 0
At reume time, MSI-X is unmasked (ie. enabled) and is configured to use:
address: 0x00000000fee01004
data: 0x000000000000402b
> [ 36.921370] r8169 0000:03:00.0: MSI-X entry: context suspend: fee01004 0 402b 0
Second suspend:
address: 0x00000000fee01004
data: 0x000000000000402b
> [ 37.239407] r8169 0000:03:00.0: MSI-X entry: context resume: fee01004 0 402b 0
Second resume:
address: 0x00000000fee01004
data: 0x000000000000402b
And this all looks normal. The data field is changing when you first up the device
and interrupts are enabled. This is where the request_irq happens, the MSI
vector is allocated, and that vector number is written to the data field of the
MSI-X entry.
It looks like this (re-)allocation of MSI vectors happens on every resume as well.
And that's why the data field changes each resume.
^ permalink raw reply
* Re: [PATCH] libbpf: Remove the duplicate checking of function storage
From: Taeung Song @ 2018-08-22 2:54 UTC (permalink / raw)
To: Daniel Borkmann, Jakub Kicinski
Cc: Alexei Starovoitov, Linux Netdev List, LKML
In-Reply-To: <e0799ee4-875d-de62-e6a1-cc461fe38be7@iogearbox.net>
On 08/22/2018 05:11 AM, Daniel Borkmann wrote:
> On 08/21/2018 06:46 PM, Jakub Kicinski wrote:
>> On Tue, Aug 21, 2018 at 6:12 PM, Taeung Song <treeze.taeung@gmail.com> wrote:
>>> After the commit eac7d84519a3 ("tools: libbpf: don't return '.text'
>>> as a program for multi-function programs"), bpf_program__next()
>>> in bpf_object__for_each_program skips the function storage such as .text,
>>> so eliminate the duplicate checking.
>>>
>>> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>>
>> Looks reasonable, but you may need to repost once bpf-next is open:
>>
>> https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
>>
>> Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>
> Agree, please resubmit once bpf-next opens up again. Thanks!
>
OK ! I will.
Thanks,
Taeung
^ permalink raw reply
* Re: [PATCH] strparser: remove any offset before parsing messages
From: Doron Roberts-Kedes @ 2018-08-22 2:33 UTC (permalink / raw)
To: Dominique Martinet
Cc: Tom Herbert, Dave Watson, David S. Miller, netdev, linux-kernel
In-Reply-To: <20180822004647.GA10656@nautica>
On Wed, Aug 22, 2018 at 02:46:47AM +0200, Dominique Martinet wrote:
> Yes, the rcv_msg callback itself can get the offset easily, and it's not
> that which needs an extra parameter but the bpf function kcm/sockmap are
> calling which would need either an extra parameter or changing to get
> that value themselves.
Ah cool. Thanks for explaining.
> For what it's worth, I don't think either are acceptable solutions, I'm
> just stating what would a "fix in bpf" would be.
Agreed that the discussion should be about whether to fix it up in
strparser or sockmap. bpf seems inappropriate.
> strparser logic in that case -- it might work to pull in the parser
> function but it might not work in rcv for all I know, or the next user
> might think that since pull is ok some other operation on the skb is as
> well...
Just to make sure I understand, is it possible you meant to say that the
other way around? Surely the rcv callback can do whatever it wants with
the skb. Its the parse callback that may need to be a little more
careful with the skb.
For the parse case, why not just clone and pull?
> As I wrote above, I think it should not be possible, so we're not
> even talking about a small percentage here.
> The reason I didn't use skb_pull (the head-only variant) is that I'd
> rather have the overhead than a BUG() if I'm wrong on this...
A printk in that section when (orig_offset + eaten > skb_headlen(head))
confirms that this case is not uncommon or impossible. Would have to do
more work to see how many hundreds of times per second, but it is not a
philosophical concern.
^ permalink raw reply
* Re: [PATCH] datapath.c: fix missing return value check of nla_nest_start()
From: Pravin Shelar @ 2018-08-21 22:38 UTC (permalink / raw)
To: jasonwood2031; +Cc: Linux Kernel Network Developers
In-Reply-To: <20180817081508.7104-1-jasonwood2031@gmail.com>
On Fri, Aug 17, 2018 at 1:15 AM Jiecheng Wu <jasonwood2031@gmail.com> wrote:
>
> Function queue_userspace_packet() defined in net/openvswitch/datapath.c calls nla_nest_start() to allocate memory for struct nlattr which is dereferenced immediately. As nla_nest_start() may return NULL on failure, this code piece may cause NULL pointer dereference bug.
> ---
> net/openvswitch/datapath.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 0f5ce77..ff4457d 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -460,6 +460,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>
> if (upcall_info->egress_tun_info) {
> nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_EGRESS_TUN_KEY);
> + if (!nla)
> + return -EMSGSIZE;
It is not possible, since user_skb is allocated to accommodate all
netlink attributes.
> err = ovs_nla_put_tunnel_info(user_skb,
> upcall_info->egress_tun_info);
> BUG_ON(err);
> @@ -468,6 +470,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>
> if (upcall_info->actions_len) {
> nla = nla_nest_start(user_skb, OVS_PACKET_ATTR_ACTIONS);
> + if (!nla)
> + return -EMSGSIZE;
same as above, the check is not required.
> err = ovs_nla_put_actions(upcall_info->actions,
> upcall_info->actions_len,
> user_skb);
> --
> 2.6.4
>
^ permalink raw reply
* [PATCH 00/11] tree-wide: use SPDX identifier for Renesas drivers
From: Wolfram Sang @ 2018-08-21 22:02 UTC (permalink / raw)
To: linux-renesas-soc
Cc: Kuninori Morimoto, Wolfram Sang, dmaengine, linux-can, linux-clk,
linux-i2c, linux-kernel, linux-mmc, linux-pwm, linux-spi,
linux-watchdog, netdev
Here is my first batch of SPDX conversion patches. Mostly low hanging fruits,
only a handful of drivers needed corrections in MODULE_LICENSE, too.
checkpatch is happy as well.
I think an ack from Morimoto-san with his Renesas email address would be nice
to make it more official? Still, I *am* contracted by Renesas.
Thanks,
Wolfram
Wolfram Sang (11):
clk: renesas: use SPDX identifier for Renesas drivers
dmaengine: use SPDX identifier for Renesas drivers
i2c: use SPDX identifier for Renesas drivers
mmc: use SPDX identifier for Renesas drivers
can: rcar: use SPDX identifier for Renesas drivers
net: ethernet: renesas: use SPDX identifier for Renesas drivers
phy: renesas: use SPDX identifier for Renesas drivers
pwm: use SPDX identifier for Renesas drivers
soc: renesas: use SPDX identifier for Renesas drivers
spi: use SPDX identifier for Renesas drivers
watchdog: use SPDX identifier for Renesas drivers
drivers/clk/renesas/clk-div6.c | 5 +----
drivers/clk/renesas/r8a7795-cpg-mssr.c | 5 +----
drivers/clk/renesas/r8a7796-cpg-mssr.c | 5 +----
drivers/clk/renesas/r8a77995-cpg-mssr.c | 5 +----
drivers/clk/renesas/rcar-gen3-cpg.c | 5 +----
drivers/clk/renesas/rcar-usb2-clock-sel.c | 5 +----
drivers/clk/renesas/renesas-cpg-mssr.c | 5 +----
drivers/clk/renesas/renesas-cpg-mssr.h | 5 +----
drivers/dma/nbpfaxi.c | 5 +----
drivers/dma/sh/shdma-arm.h | 5 +----
drivers/dma/sh/shdma-base.c | 5 +----
drivers/dma/sh/shdma-of.c | 5 +----
drivers/dma/sh/shdma-r8a73a4.c | 5 +----
drivers/dma/sh/shdma.h | 6 +-----
drivers/dma/sh/shdmac.c | 6 +-----
drivers/dma/sh/sudmac.c | 5 +----
drivers/dma/sh/usb-dmac.c | 5 +----
drivers/i2c/busses/i2c-emev2.c | 5 +----
drivers/i2c/busses/i2c-highlander.c | 5 +----
drivers/i2c/busses/i2c-rcar.c | 10 +---------
drivers/i2c/busses/i2c-riic.c | 5 +----
drivers/i2c/busses/i2c-sh_mobile.c | 10 +---------
drivers/mmc/host/renesas_sdhi.h | 5 +----
drivers/mmc/host/renesas_sdhi_core.c | 5 +----
drivers/mmc/host/renesas_sdhi_internal_dmac.c | 5 +----
drivers/mmc/host/renesas_sdhi_sys_dmac.c | 5 +----
drivers/mmc/host/sh_mmcif.c | 7 ++-----
drivers/mmc/host/tmio_mmc.c | 5 +----
drivers/mmc/host/tmio_mmc.h | 6 +-----
drivers/mmc/host/tmio_mmc_core.c | 5 +----
drivers/mmc/host/usdhi6rol0.c | 5 +----
drivers/net/can/rcar/rcar_can.c | 6 +-----
drivers/net/can/rcar/rcar_canfd.c | 6 +-----
drivers/net/ethernet/renesas/ravb.h | 5 +----
drivers/net/ethernet/renesas/ravb_main.c | 5 +----
drivers/net/ethernet/renesas/sh_eth.c | 13 +------------
drivers/net/ethernet/renesas/sh_eth.h | 13 +------------
drivers/phy/renesas/phy-rcar-gen2.c | 5 +----
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 5 +----
drivers/phy/renesas/phy-rcar-gen3-usb3.c | 5 +----
drivers/pwm/pwm-rcar.c | 5 +----
drivers/pwm/pwm-renesas-tpu.c | 10 +---------
drivers/soc/renesas/r8a7743-sysc.c | 5 +----
drivers/soc/renesas/r8a7745-sysc.c | 5 +----
drivers/soc/renesas/r8a7779-sysc.c | 5 +----
drivers/soc/renesas/r8a7790-sysc.c | 5 +----
drivers/soc/renesas/r8a7791-sysc.c | 5 +----
drivers/soc/renesas/r8a7792-sysc.c | 5 +----
drivers/soc/renesas/r8a7794-sysc.c | 5 +----
drivers/soc/renesas/r8a7795-sysc.c | 5 +----
drivers/soc/renesas/r8a7796-sysc.c | 5 +----
drivers/soc/renesas/r8a77970-sysc.c | 5 +----
drivers/soc/renesas/r8a77995-sysc.c | 5 +----
drivers/soc/renesas/rcar-sysc.h | 5 +----
drivers/soc/renesas/renesas-soc.c | 10 +---------
drivers/spi/spi-rspi.c | 10 +---------
drivers/spi/spi-sh-hspi.c | 12 ++----------
drivers/spi/spi-sh-msiof.c | 6 +-----
drivers/spi/spi-sh.c | 12 ++----------
drivers/watchdog/renesas_wdt.c | 5 +----
60 files changed, 63 insertions(+), 300 deletions(-)
--
2.11.0
^ permalink raw reply
* [PATCH bpf] bpf, sockmap: fix sock_hash_alloc and reject zero-sized keys
From: Daniel Borkmann @ 2018-08-21 22:02 UTC (permalink / raw)
To: alexei.starovoitov; +Cc: john.fastabend, netdev, Daniel Borkmann
Currently, it is possible to create a sock hash map with key size
of 0 and have the kernel return a fd back to user space. This is
invalid for hash maps (and kernel also hasn't been tested for zero
key size support in general at this point). Thus, reject such
configuration.
Fixes: 81110384441a ("bpf: sockmap, add hash map support")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: John Fastabend <john.fastabend@gmail.com>
---
kernel/bpf/sockmap.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 98e621a..60ceb0e 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -2140,7 +2140,9 @@ static struct bpf_map *sock_hash_alloc(union bpf_attr *attr)
return ERR_PTR(-EPERM);
/* check sanity of attributes */
- if (attr->max_entries == 0 || attr->value_size != 4 ||
+ if (attr->max_entries == 0 ||
+ attr->key_size == 0 ||
+ attr->value_size != 4 ||
attr->map_flags & ~SOCK_CREATE_FLAG_MASK)
return ERR_PTR(-EINVAL);
--
2.9.5
^ permalink raw reply related
* Re: Experimental fix for MSI-X issue on r8169
From: Steve Dodd @ 2018-08-21 21:48 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Jian-Hong Pan, Lou Reed, netdev@vger.kernel.org
In-Reply-To: <98df6b0a-44df-9c0d-ddf6-11b892ccd990@gmail.com>
On 21 August 2018 at 22:19, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Below is a patch printing the MSI-X table entry in different contexts,
> it's not supposed to fix anything. Could you please let me know
> what the output is on your system?
> I want to get an idea whether the issue clears the complete entry or
> just corrupts certain parts.
Here we go..
[ 2.865434] r8169 0000:01:00.0: MSI-X entry: context probe: fee01004 0 40ef 1
[ 269.747608] r8169 0000:01:00.0: MSI-X entry: context probe: fee01004 0 40ef 1
[ 295.454099] r8169 0000:01:00.0: MSI-X entry: context suspend:
fee08004 0 4028 0
S.
^ permalink raw reply
* Re: [PATCH] strparser: remove any offset before parsing messages
From: Dominique Martinet @ 2018-08-22 0:46 UTC (permalink / raw)
To: Doron Roberts-Kedes
Cc: Tom Herbert, Dave Watson, David S. Miller, netdev, linux-kernel
In-Reply-To: <20180821233549.GA96607@doronrk-mbp.dhcp.thefacebook.com>
Doron Roberts-Kedes wrote on Tue, Aug 21, 2018:
> On Wed, Aug 22, 2018 at 12:51:13AM +0200, Dominique Martinet wrote:
> > That's maybe three more lines than the current patch, which is also
> > pretty simple, I'm not sure what you're expecting from alternative
> > solutions to call that overly complicated...
>
> The line count is not the source of the complexity. The undue complexity
> is having strparser operate in two modes: one for clients that properly
> use the API by respecting the value of offset, and another for clients
> that do not.
I might sound stubborn at this point but that still does not sound
complex compared to the alternatives I can think of.
> > I don't think bpf itself needs to be changed here -- the offset is
> > stored in a strparser specific struct so short of such a skb_pull I
> > think we'd need to change the type of the bpf function, pass it it the
> > extra parameter, and make it a user visible change breaking the kcm
> > API... And I have no idea for sockmap but probably something similar.
>
> I'm not sure I follow you here. Any rcv_msg callback implementation
> receives an skb. Calling strp_msg() on the skb gives you the strp_msg
> which has the offset value. Can you explain why passing an extra
> parameter is necessary to get the offset?
Yes, the rcv_msg callback itself can get the offset easily, and it's not
that which needs an extra parameter but the bpf function kcm/sockmap are
calling which would need either an extra parameter or changing to get
that value themselves -- the later is probably not very difficult and
won't break the API, but at the same time pushes the responsability to
kcm/sockmap users who will get that wrong and waste time trying to
understand how kcm works like I did.
Breaking the API has the advantage that users will get an error telling
them to update their bpf program instead of randomly failing when tcp
aggregation happens.
For what it's worth, I don't think either are acceptable solutions, I'm
just stating what would a "fix in bpf" would be.
A "fix in kcm/sockmap" would be to pull just before calling the bpf
program, which could be a middle ground, but I think that's more
perverse than adding a flag to strparser for new users because we'd
basically be saying users should modify the skb that strparser users
under its feet, and there is no guarantee what would happen to the
strparser logic in that case -- it might work to pull in the parser
function but it might not work in rcv for all I know, or the next user
might think that since pull is ok some other operation on the skb is as
well...
> > (Also, note that pskb_pull will not copy any data or allocate memory
> > unless we're pulling past the end of the skb, which seems pretty
> > unlikely in that situation as we should have consumed any fully "eaten"
> > skb before getting to a new one here -- so in practice this patch just
> > adds a skb->data += offset with safety guards "just in case")
>
> Yes, no data will be copied if the you don't pull beyond the linear
> buffer. Adding overhead even in a small percentage of cases still
> requires a good justification.
As I wrote above, I think it should not be possible, so we're not
even talking about a small percentage here.
The reason I didn't use skb_pull (the head-only variant) is that I'd
rather have the overhead than a BUG() if I'm wrong on this...
> In this particular case, I think a good justification would be
> demonstrating that it is impractical for the buggy strparser users
> you've pointed out to use the existing API and respect the value of
> offset.
That's what the previous paragraph about changing the API intended to
do.
> You have indicated that you are not super familiar with the bpf code,
> which is fine (I'm not either), but this isn't a good reason to make a
> change to strparser instead of bpf.
I didn't even know kcm/strparser existed a few weeks ago, not being
familiar doesn't mean I didn't look at how it works.
I'd be glad to be proven wrong here but I really do not think there is a
possibility within the bpf framework to give a fake skb with an external
offset to the bpf program transparently.
Assuming there is, it would have to be more expensive than a pull (it'd
basically need to do a pull then restore the original data somehow)...
And if there isn't I certainly do not want to add one, not because I
don't want to mess with something I'm not too familiar with (that'd
actually be an argument to do it that way to me), but because I do not
think it belongs there; bpf should not need to know what a skb is or how
it works.
All being said I'm still convinced having two modes in strparser is the
simplest solution.
--
Dominique
^ permalink raw reply
* Re: Experimental fix for MSI-X issue on r8169
From: Heiner Kallweit @ 2018-08-21 21:19 UTC (permalink / raw)
To: Jian-Hong Pan; +Cc: Steve Dodd, Lou Reed, netdev@vger.kernel.org
In-Reply-To: <CAPpJ_ecsMOL23VYM2juZ9R8JLrSh1bjCet16XCSpv0mDaSYu6w@mail.gmail.com>
On 20.08.2018 05:47, Jian-Hong Pan wrote:
> 2018-08-20 4:34 GMT+08:00 Heiner Kallweit <hkallweit1@gmail.com>:
>> The three of you reported an MSI-X-related error when the system
>> resumes from suspend. This has been fixed for now by disabling MSI-X
>> on certain chip versions. However more versions may be affected.
>>
>> I checked with Realtek and they confirmed that on certain chip
>> versions a MSIX-related value in PCI config space is reset when
>> resuming from S3.
>>
>> I would appreciate if you could test the following experimental patch
>> and whether warning "MSIX address lost, re-configuring" appears in
>> your dmesg output after resume from suspend.
>>
>> Thanks a lot for your efforts.
>
> Tested with the experiment patch on ASUS X441UAR.
>
> This is the information before suspend:
>
> dev@endless:~$ dmesg | grep r8169
> [ 10.279565] libphy: r8169: probed
> [ 10.279947] r8169 0000:02:00.0 eth0: RTL8106e, 0c:9d:92:32:67:b4,
> XID 44900000, IRQ 127
> [ 10.445952] r8169 0000:02:00.0 enp2s0: renamed from eth0
> [ 15.676229] Generic PHY r8169-200:00: attached PHY driver [Generic
> PHY] (mii_bus:phy_addr=r8169-200:00, irq=IGNORE)
> [ 17.455392] r8169 0000:02:00.0 enp2s0: Link is Up - 100Mbps/Full -
> flow control off
>
> dev@endless:~$ ip addr show enp2s0
> 4: enp2s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
> state UP group default qlen 1000
> link/ether 0c:9d:92:32:67:b4 brd ff:ff:ff:ff:ff:ff
> inet 10.100.13.152/24 brd 10.100.13.255 scope global noprefixroute
> dynamic enp2s0
> valid_lft 86347sec preferred_lft 86347sec
> inet6 fe80::2873:a2a9:6ca1:c79d/64 scope link noprefixroute
> valid_lft forever preferred_lft forever
>
> This is the information after resume:
>
> dev@endless:~$ dmesg | grep r8169
> [ 10.279565] libphy: r8169: probed
> [ 10.279947] r8169 0000:02:00.0 eth0: RTL8106e, 0c:9d:92:32:67:b4,
> XID 44900000, IRQ 127
> [ 10.445952] r8169 0000:02:00.0 enp2s0: renamed from eth0
> [ 15.676229] Generic PHY r8169-200:00: attached PHY driver [Generic
> PHY] (mii_bus:phy_addr=r8169-200:00, irq=IGNORE)
> [ 17.455392] r8169 0000:02:00.0 enp2s0: Link is Up - 100Mbps/Full -
> flow control off
> [ 95.594265] r8169 0000:02:00.0 enp2s0: Link is Down
> [ 96.242074] Generic PHY r8169-200:00: attached PHY driver [Generic
> PHY] (mii_bus:phy_addr=r8169-200:00, irq=IGNORE)
>
> dev@endless:~$ ip addr show enp2s0
> 4: enp2s0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc
> pfifo_fast state DOWN group default qlen 1000
> link/ether 0c:9d:92:32:67:b4 brd ff:ff:ff:ff:ff:ff
>
> There is no "MSIX address lost, re-configuring" in dmesg.
> The ethernet interface is still down after resume.
>
Thanks a lot for testing. Unfortunately I don't have test hardware
affected by this MSI-X issue, so maybe you can help me to understand
the issue a little better.
Below is a patch printing the MSI-X table entry in different contexts,
it's not supposed to fix anything. Could you please let me know
what the output is on your system?
I want to get an idea whether the issue clears the complete entry or
just corrupts certain parts.
That's what I get on my system (RTL8168E-VL). In your case you'll come
only till the first suspend.
[ 3.743404] r8169 0000:03:00.0: MSI-X entry: context probe: fee01004 0 40ef 1
[ 29.539250] r8169 0000:03:00.0: MSI-X entry: context suspend: fee02004 0 4028 0
[ 29.837457] r8169 0000:03:00.0: MSI-X entry: context resume: fee01004 0 402b 0
[ 36.921370] r8169 0000:03:00.0: MSI-X entry: context suspend: fee01004 0 402b 0
[ 37.239407] r8169 0000:03:00.0: MSI-X entry: context resume: fee01004 0 402b 0
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 54f53c8c0..f32645119 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -11,6 +11,7 @@
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/pci.h>
+#include <linux/msi.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/delay.h>
@@ -6822,6 +6823,20 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
pm_runtime_put_noidle(&pdev->dev);
}
+static void rtl_print_msix_entry(struct rtl8169_private *tp, const char *context)
+{
+ struct msi_desc *desc = first_pci_msi_entry(tp->pci_dev);
+ u32 data[4];
+
+ data[0] = readl(desc->mask_base + PCI_MSIX_ENTRY_LOWER_ADDR);
+ data[1] = readl(desc->mask_base + PCI_MSIX_ENTRY_UPPER_ADDR);
+ data[2] = readl(desc->mask_base + PCI_MSIX_ENTRY_DATA);
+ data[3] = readl(desc->mask_base + PCI_MSIX_ENTRY_VECTOR_CTRL);
+
+ dev_info(tp_to_dev(tp), "MSI-X entry: context %s: %x %x %x %x\n",
+ context, data[0], data[1], data[2], data[3]);
+}
+
static void rtl8169_net_suspend(struct net_device *dev)
{
struct rtl8169_private *tp = netdev_priv(dev);
@@ -6846,9 +6861,12 @@ static int rtl8169_suspend(struct device *device)
{
struct pci_dev *pdev = to_pci_dev(device);
struct net_device *dev = pci_get_drvdata(pdev);
+ struct rtl8169_private *tp = netdev_priv(dev);
rtl8169_net_suspend(dev);
+ rtl_print_msix_entry(tp, "suspend");
+
return 0;
}
@@ -6875,6 +6893,9 @@ static int rtl8169_resume(struct device *device)
{
struct pci_dev *pdev = to_pci_dev(device);
struct net_device *dev = pci_get_drvdata(pdev);
+ struct rtl8169_private *tp = netdev_priv(dev);
+
+ rtl_print_msix_entry(tp, "resume");
if (netif_running(dev))
__rtl8169_resume(dev);
@@ -7075,11 +7096,6 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
RTL_W8(tp, Config2, RTL_R8(tp, Config2) & ~MSIEnable);
RTL_W8(tp, Cfg9346, Cfg9346_Lock);
flags = PCI_IRQ_LEGACY;
- } else if (tp->mac_version == RTL_GIGA_MAC_VER_40) {
- /* This version was reported to have issues with resume
- * from suspend when using MSI-X
- */
- flags = PCI_IRQ_LEGACY | PCI_IRQ_MSI;
} else {
flags = PCI_IRQ_ALL_TYPES;
}
@@ -7354,6 +7370,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
return rc;
}
+ rtl_print_msix_entry(tp, "probe");
+
tp->saved_wolopts = __rtl8169_get_wol(tp);
mutex_init(&tp->wk.mutex);
--
2.18.0
^ permalink raw reply related
* Re: [PATCH] strparser: remove any offset before parsing messages
From: Doron Roberts-Kedes @ 2018-08-21 21:15 UTC (permalink / raw)
To: Dominique Martinet
Cc: Tom Herbert, Dave Watson, David S. Miller, netdev, linux-kernel
In-Reply-To: <20180821193655.GA15354@nautica>
On Tue, Aug 21, 2018 at 09:36:55PM +0200, Dominique Martinet wrote:
> One of the solutions I had suggested was adding a flag at strparser
> setup time to only do that pull for users which cannot handle offset,
> but nobody seemed interested two weeks ago. I can still do that.
This seems overly complicated.
> That's still suboptimal, but I don't have any better idea.
> To properly fix the users, I'd really need help with how bpf works to
> even know if passing an offset would be possible in the first place, as
> I do not see how at this time.
Thanks for clarifying Dominique.
It seems like we mainly agree that the proposed patch is suboptimal for
existing clients of the library that use offset correctly (tls).
It also seems like you've identified that the proper fix is in bpf.
Regrettably, I cannot help you understand how bpf works because I'm not
familiar with that code.
As an aside, I would recommend reaching the netdev FAQ page:
https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
It contains helpful hints about how to format email subjects (specifying
net vs. net-next) and determining when trees are closed (currently
closed).
^ permalink raw reply
* Re: [PATCH] strparser: remove any offset before parsing messages
From: Doron Roberts-Kedes @ 2018-08-21 23:35 UTC (permalink / raw)
To: Dominique Martinet
Cc: Tom Herbert, Dave Watson, David S. Miller, netdev, linux-kernel
In-Reply-To: <20180821225113.GA6515@nautica>
On Wed, Aug 22, 2018 at 12:51:13AM +0200, Dominique Martinet wrote:
> That's maybe three more lines than the current patch, which is also
> pretty simple, I'm not sure what you're expecting from alternative
> solutions to call that overly complicated...
The line count is not the source of the complexity. The undue complexity
is having strparser operate in two modes: one for clients that properly
use the API by respecting the value of offset, and another for clients
that do not.
> I don't think bpf itself needs to be changed here -- the offset is
> stored in a strparser specific struct so short of such a skb_pull I
> think we'd need to change the type of the bpf function, pass it it the
> extra parameter, and make it a user visible change breaking the kcm
> API... And I have no idea for sockmap but probably something similar.
I'm not sure I follow you here. Any rcv_msg callback implementation
receives an skb. Calling strp_msg() on the skb gives you the strp_msg
which has the offset value. Can you explain why passing an extra
parameter is necessary to get the offset?
> I can't think of that as better than adding a flag to strparser.
>
> (Also, note that pskb_pull will not copy any data or allocate memory
> unless we're pulling past the end of the skb, which seems pretty
> unlikely in that situation as we should have consumed any fully "eaten"
> skb before getting to a new one here -- so in practice this patch just
> adds a skb->data += offset with safety guards "just in case")
Yes, no data will be copied if the you don't pull beyond the linear
buffer. Adding overhead even in a small percentage of cases still
requires a good justification. In this particular case, I think a good
justification would be demonstrating that it is impractical for the
buggy strparser users you've pointed out to use the existing API and
respect the value of offset. You have indicated that you are not super
familiar with the bpf code, which is fine (I'm not either), but this
isn't a good reason to make a change to strparser instead of bpf.
^ permalink raw reply
* Re: [PATCH bpf] xsk: fix return value of xdp_umem_assign_dev()
From: Daniel Borkmann @ 2018-08-21 20:12 UTC (permalink / raw)
To: Björn Töpel, bhole_prashant_q7
Cc: ast, Björn Töpel, Karlsson, Magnus, David Miller,
Netdev
In-Reply-To: <CAJ+HfNh88W3gODra0Uh2BSSAVhg=bka_MyA9=9J8QuEep16VWA@mail.gmail.com>
On 08/20/2018 10:31 AM, Björn Töpel wrote:
> Den mån 20 aug. 2018 kl 02:58 skrev Prashant Bhole
> <bhole_prashant_q7@lab.ntt.co.jp>:
>>
>> s/ENOTSUPP/EOPNOTSUPP/ in function umem_assign_dev().
>> This function's return value is directly returned by xsk_bind().
>> EOPNOTSUPP is bind()'s possible return value.
>>
>> Fixes: f734607e819b ("xsk: refactor xdp_umem_assign_dev()")
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
Applied to bpf, thanks!
^ permalink raw reply
* Re: [Patch net 0/9] net_sched: pending clean up and bug fixes
From: David Miller @ 2018-08-21 20:00 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, jhs
In-Reply-To: <20180819192213.14196-1-xiyou.wangcong@gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Sun, 19 Aug 2018 12:22:04 -0700
> This patchset aims to clean up and fixes some bugs in current
> merge window, this is why it is targeting -net.
>
> Patch 1-5 are clean up Vlad's patches merged in current merge
> window, patch 6 is just a trivial cleanup.
>
> Patch 7 reverts a lockdep warning fix and patch 8 provides a better
> fix for it.
>
> Patch 9 fixes a potential deadlock found by me during code review.
>
> Please see each patch for details.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Series applied and patches #8 and #9 queued up for -stable.
^ permalink raw reply
* Re: [Patch net 8/9] act_ife: move tcfa_lock down to where necessary
From: David Miller @ 2018-08-21 19:43 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, jhs, vladbu
In-Reply-To: <CAM_iQpXR56EUwF77QETJvKAvA6mHR3kNOEdA0CuF+uyhr+_Frg@mail.gmail.com>
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 20 Aug 2018 16:57:46 -0700
> Passing 'exists' as 'atomic' is prior to my change. With my change,
> they are separated as two parameters:
I mis-read the patch, thanks for explaining :)
^ permalink raw reply
* Re: [PATCH] strparser: remove any offset before parsing messages
From: Dominique Martinet @ 2018-08-21 22:51 UTC (permalink / raw)
To: Doron Roberts-Kedes
Cc: Tom Herbert, Dave Watson, David S. Miller, netdev, linux-kernel
In-Reply-To: <20180821211504.GA76892@doronrk-mbp.dhcp.thefacebook.com>
Doron Roberts-Kedes wrote on Tue, Aug 21, 2018:
> On Tue, Aug 21, 2018 at 09:36:55PM +0200, Dominique Martinet wrote:
> > One of the solutions I had suggested was adding a flag at strparser
> > setup time to only do that pull for users which cannot handle offset,
> > but nobody seemed interested two weeks ago. I can still do that.
>
> This seems overly complicated.
That sounds much simpler than adding a similar feature to bpf if it
doesn't already support it -- we already have an user-provided struct at
strparser init timer (strp->cb) in which it would be easy to add a flag
saying whether the callbacks support offset or not.
That's maybe three more lines than the current patch, which is also
pretty simple, I'm not sure what you're expecting from alternative
solutions to call that overly complicated...
> It seems like we mainly agree that the proposed patch is suboptimal for
> existing clients of the library that use offset correctly (tls).
>
> It also seems like you've identified that the proper fix is in bpf.
I don't think bpf itself needs to be changed here -- the offset is
stored in a strparser specific struct so short of such a skb_pull I
think we'd need to change the type of the bpf function, pass it it the
extra parameter, and make it a user visible change breaking the kcm
API... And I have no idea for sockmap but probably something similar.
I can't think of that as better than adding a flag to strparser.
(Also, note that pskb_pull will not copy any data or allocate memory
unless we're pulling past the end of the skb, which seems pretty
unlikely in that situation as we should have consumed any fully "eaten"
skb before getting to a new one here -- so in practice this patch just
adds a skb->data += offset with safety guards "just in case")
> As an aside, I would recommend reaching the netdev FAQ page:
> https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt
>
> It contains helpful hints about how to format email subjects (specifying
> net vs. net-next) and determining when trees are closed (currently
> closed).
Thank you for pointing out to that document.
While this bug has been around from day one of kcm it is still a bug fix
so I'd rather let maintainers decide which tree it goes to.
I wasn't aware that net-next closes during the Linus merge window, but
if you want to split hairs, the FAQ says "do not send new net-next
content to netdev [while closed]" and this isn't new content as the v0
of the patch was mostly the same and sent before the 4.18 release, (and,
ironically, did not get any comment).
Anyway, sure, I'll wait until next week for a v2 if that matters.
Thanks,
--
Dominique
^ permalink raw reply
* Re: [PATCH 05/11] can: rcar: use SPDX identifier for Renesas drivers
From: Fabio Estevam @ 2018-08-21 22:45 UTC (permalink / raw)
To: Wolfram Sang, Philippe Ombredanne
Cc: Linux-Renesas, Kuninori Morimoto, Wolfgang Grandegger,
Marc Kleine-Budde, David S. Miller, linux-can, netdev,
linux-kernel
In-Reply-To: <20180821220233.9202-6-wsa+renesas@sang-engineering.com>
Hi Wolfram,
On Tue, Aug 21, 2018 at 7:02 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> To be applied individually per subsystem tree. Morimoto-san, could you maybe
> ack this with your Renesas address?
>
> drivers/net/can/rcar/rcar_can.c | 6 +-----
> drivers/net/can/rcar/rcar_canfd.c | 6 +-----
> 2 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/can/rcar/rcar_can.c b/drivers/net/can/rcar/rcar_can.c
> index 11662f479e76..051bf4ef4be2 100644
> --- a/drivers/net/can/rcar/rcar_can.c
> +++ b/drivers/net/can/rcar/rcar_can.c
> @@ -1,12 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
According to Documentation/process/license-rules.rst the format should
be like this instead:
// SPDX-License-Identifier: GPL-2.0+
^ permalink raw reply
* Re: [PATCH net] hv_netvsc: ignore devices that are not PCI
From: David Miller @ 2018-08-21 19:03 UTC (permalink / raw)
To: stephen; +Cc: kys, netdev, sthemmin
In-Reply-To: <20180821174038.7942-1-sthemmin@microsoft.com>
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Tue, 21 Aug 2018 10:40:38 -0700
> Registering another device with same MAC address (such as TAP, VPN or
> DPDK KNI) will confuse the VF autobinding logic. Restrict the search
> to only run if the device is known to be a PCI attached VF.
>
> Fixes: e8ff40d4bff1 ("hv_netvsc: improve VF device matching")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
Applied and queued up for -stable.
^ 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