* Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
From: Sekhar Nori @ 2017-10-18 13:24 UTC (permalink / raw)
To: Marc Kleine-Budde, Franklin S Cooper Jr, Mario Hüttel,
Yang, Wenyou, wg, socketcan, quentin.schulz, edumazet, linux-can,
netdev, linux-kernel
Cc: Wenyou Yang, Dong Aisheng, Quadros, Roger
In-Reply-To: <08432016-e733-b827-b9aa-bc359dd837f5@pengutronix.de>
Hi Marc,
On Wednesday 18 October 2017 06:14 PM, Marc Kleine-Budde wrote:
> On 09/21/2017 02:48 AM, Franklin S Cooper Jr wrote:
>>
>>
>> On 09/20/2017 04:37 PM, Mario Hüttel wrote:
>>>
>>>
>>> On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote:
>>>> Hi Wenyou,
>>>>
>>>> On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>>>>>
>>>>> On 2017/9/14 13:06, Sekhar Nori wrote:
>>>>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>>>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>>>>>> bit
>>>>>>>> was being transmitted and with a bit more investigation realized the
>>>>>>>> actual
>>>>>>>> MCAN IP would go back to initialization mode automatically.
>>>>>>>>
>>>>>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>>>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>>>>>> Compensation Offset register should be set. The document mentions
>>>>>>>> that this
>>>>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>>>>>
>>>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>>>>>> indicates
>>>>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>>>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>>>>>> rate
>>>>>>>> is above 2.5 Mbps.
>>>>>>>>
>>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>>>>> ---
>>>>>>>> I'm pretty surprised that this hasn't been implemented already since
>>>>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>>>>>> supports up to 10 Mbps.
>>>>>>>>
>>>>>>>> So it will be nice to get comments from users of this driver to
>>>>>>>> understand
>>>>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>>>>>> patch.
>>>>>>>> If they haven't what did they do to get around it if they needed higher
>>>>>>>> speeds.
>>>>>>>>
>>>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>>>>>> insure
>>>>>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>>>>>> transceiver.
>>>>>>> ping. Anyone has any thoughts on this?
>>>>>> I added Dong who authored the m_can driver and Wenyou who added the only
>>>>>> in-kernel user of the driver for any help.
>>>>> I tested it on SAMA5D2 Xplained board both with and without this patch,
>>>>> both work with the 4M bps data bit rate.
>>>> Thank you for testing this out. Its interesting that you have been able
>>>> to use higher speeds without this patch. What is the CAN transceiver
>>>> being used on the SAMA5D2 Xplained board? I tried looking at the
>>>> schematic but it seems the CAN signals are used on an extension board
>>>> which I can't find the schematic for. Also do you mind sharing your test
>>>> setup? Were you doing a short point to point test?
>>>>
>>>> Thank You,
>>>> Franklin
>>> Hello Franklin,
>>>
>>> your patch definitely makes sense.
>>>
>>> I forgot the TDC in my patches because it was not present in the
>>> previous driver versions and because I didn't encounter any
>>> problems when testing it myself.
>>>
>>> The error is highly dependent on the hardware (transceiver) setup.
>>> So it is definitely possible that some people don't encounter errors
>>> without your patch.
>>
>> So the Transmission Delay Compensation feature Value register is suppose
>> to take into consideration the transceiver delay automatically and add
>> the value of TDCO on top of that. So why would TDCO be dependent on the
>> transceiver? I've heard conflicting things regarding TDC so any
>> clarification on what actually impacts it would be appreciated.
>>
>> Also part of the issue I'm having is how can we properly configure TDCO?
>> Configuring TDCO is essentially figuring out what Secondary Sample Point
>> to use. However, it is unclear what value to set SSP to and which use
>> cases a given SSP will work or doesn't work. I've seen various
>> recommendations from Bosch on choosing SSP but ultimately it seems they
>> suggestion "real world testing" to come up with a proper value. Not
>> setting TDCO causes problems for my device and improperly setting TDCO
>> causes problems for my device. So its likely any value I use could end
>> up breaking something for someone else.
>>
>> Currently I leaning to a DT property that can be used for setting SSP.
>> Perhaps use a generic default value and allow individuals to override it
>> via DT?
>
> Sounds reasonable. What's the status of this series?
I have had some offline discussions with Franklin on this, and I am not
fully convinced that DT is the way to go here (although I don't have the
agreement with Franklin there).
There are two components in configuring the secondary sample point. It
is the transceiver loopback delay and an offset (example half of the bit
time in data phase).
While the transceiver loopback delay is pretty board dependent (and thus
amenable to DT encoding), I am not quite sure the offset can be
configured in DT because its not really board dependent.
Unfortunately, offset calculation does not seem to be an exact science.
There are recommendations ranging from using 50% of bit time to making
it same as the sample point configured. This means users who need to
change the SSP due to offset variations need to change their DT even
without anything changing on their board.
Since we have a netlink socket interface to configure sample point, I
wonder if that should be extended to configure SSP too (or at least the
offset part of SSP)?
Thanks,
Sekhar
^ permalink raw reply
* Re: [PATCH net 0/3] Fix for BPF devmap percpu allocation splat
From: Tejun Heo @ 2017-10-18 13:25 UTC (permalink / raw)
To: Daniel Borkmann
Cc: davem, ast, john.fastabend, mark.rutland, richard, sp3485, netdev,
linux-kernel, Dennis Zhou
In-Reply-To: <cover.1508251210.git.daniel@iogearbox.net>
Hello, Daniel.
(cc'ing Dennis)
On Tue, Oct 17, 2017 at 04:55:51PM +0200, Daniel Borkmann wrote:
> The set fixes a splat in devmap percpu allocation when we alloc
> the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
> patch 1 is rather small, so if this could be routed via -net, for
> example, with Tejun's Ack that would be good. Patch 3 gets rid of
> remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
> internals and should not be used.
>
> Thanks!
>
> Daniel Borkmann (3):
> mm, percpu: add support for __GFP_NOWARN flag
This looks fine.
> bpf: fix splat for illegal devmap percpu allocation
> bpf: do not test for PCPU_MIN_UNIT_SIZE before percpu allocations
These look okay too but if it helps percpu allocator can expose the
maximum size / alignment supported to take out the guessing game too.
Also, the reason why PCPU_MIN_UNIT_SIZE is what it is is because
nobody needed anything bigger. Increasing the size doesn't really
cost much at least on 64bit archs. Is that something we want to be
considering?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH net] sock_diag: request _diag module only when the family or proto has been registered
From: Xin Long @ 2017-10-18 13:33 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, network dev, davem, Marcelo Ricardo Leitner,
Sabrina Dubroca, Eric Dumazet
In-Reply-To: <201710181738.Tq0zphPS%fengguang.wu@intel.com>
On Wed, Oct 18, 2017 at 5:38 PM, kbuild test robot <lkp@intel.com> wrote:
> Hi Xin,
>
> [auto build test ERROR on net/master]
>
> url: https://github.com/0day-ci/linux/commits/Xin-Long/sock_diag-request-_diag-module-only-when-the-family-or-proto-has-been-registered/20171018-152434
> config: x86_64-randconfig-a0-10181611 (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All errors (new ones prefixed by >>):
>
> net/core/sock_diag.o: In function `sock_diag_request_module':
>>> (.text+0x685): undefined reference to `inet_protos'
Huh,
# CONFIG_INET is not set
Cool, I will move the check for inet_protos to net_diag.ko
and repost.
Thanks.
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* [PATCH net] sctp: add the missing sock_owned_by_user check in sctp_icmp_redirect
From: Xin Long @ 2017-10-18 13:37 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Eric Dumazet, Marcelo Ricardo Leitner, Neil Horman
Now sctp processes icmp redirect packet in sctp_icmp_redirect where
it calls sctp_transport_dst_check in which tp->dst can be released.
The problem is before calling sctp_transport_dst_check, it doesn't
check sock_owned_by_user, which means tp->dst could be freed while
a process is accessing it with owning the socket.
An use-after-free issue could be triggered by this.
This patch is to fix it by checking sock_owned_by_user before calling
sctp_transport_dst_check in sctp_icmp_redirect, so that it would not
release tp->dst if users still hold sock lock.
Besides, the same issue fixed in commit 45caeaa5ac0b ("dccp/tcp: fix
routing redirect race") on sctp also needs this check.
Fixes: 55be7a9c6074 ("ipv4: Add redirect support to all protocol icmp error handlers")
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/input.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sctp/input.c b/net/sctp/input.c
index 92a0714..34f10e7 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -421,7 +421,7 @@ void sctp_icmp_redirect(struct sock *sk, struct sctp_transport *t,
{
struct dst_entry *dst;
- if (!t)
+ if (sock_owned_by_user(sk) || !t)
return;
dst = sctp_transport_dst_check(t);
if (dst)
--
2.1.0
^ permalink raw reply related
* Re: [PATCH net-next v6 1/5] bpf: Add file mode configuration into bpf maps
From: Daniel Borkmann @ 2017-10-18 13:52 UTC (permalink / raw)
To: Chenbo Feng, netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Stephen Smalley, James Morris, Paul Moore, Chenbo Feng
In-Reply-To: <20171016191135.8046-2-chenbofeng.kernel@gmail.com>
Hey Chenbo,
there's still one thing I noticed later one; would have sent a
follow-up, but as you need to respin anyway for the build issue,
here's what is still missing uapi-wise:
On 10/16/2017 09:11 PM, Chenbo Feng wrote:
[...]
> +int bpf_get_file_flag(int flags)
> +{
> + if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY))
> + return -EINVAL;
> + if (flags & BPF_F_RDONLY)
> + return O_RDONLY;
> + if (flags & BPF_F_WRONLY)
> + return O_WRONLY;
> + return O_RDWR;
> }
[...]
> -#define BPF_OBJ_LAST_FIELD bpf_fd
> +#define BPF_OBJ_LAST_FIELD file_flags
>
> static int bpf_obj_pin(const union bpf_attr *attr)
> {
> - if (CHECK_ATTR(BPF_OBJ))
> + if (CHECK_ATTR(BPF_OBJ) || attr->file_flags != 0)
> return -EINVAL;
>
> return bpf_obj_pin_user(attr->bpf_fd, u64_to_user_ptr(attr->pathname));
> @@ -1126,7 +1184,8 @@ static int bpf_obj_get(const union bpf_attr *attr)
> if (CHECK_ATTR(BPF_OBJ) || attr->bpf_fd != 0)
Here, we also need to check and bail out on ...
attr->file_flags & ~(BPF_F_RDONLY | BPF_F_WRONLY)
... otherwise we cannot extend it with more flags in future. Basically
same principle for mask check you do on map creation, but not yet here.
The same is needed in bpf_map_get_fd_by_id(), too.
The bpf_prog_get_fd_by_id() is covered since BPF_PROG_GET_FD_BY_ID_LAST_FIELD
still points to prog_id, so ok.
> return -EINVAL;
> - return bpf_obj_get_user(u64_to_user_ptr(attr->pathname));
> + return bpf_obj_get_user(u64_to_user_ptr(attr->pathname),
> + attr->file_flags);
> }
>
^ permalink raw reply
* Re: [PATCH net] sctp: add the missing sock_owned_by_user check in sctp_icmp_redirect
From: Marcelo Ricardo Leitner @ 2017-10-18 13:56 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Eric Dumazet, Neil Horman
In-Reply-To: <7f2a5122d93d4b72115027690e89c0a164097452.1508333869.git.lucien.xin@gmail.com>
On Wed, Oct 18, 2017 at 09:37:49PM +0800, Xin Long wrote:
> Now sctp processes icmp redirect packet in sctp_icmp_redirect where
> it calls sctp_transport_dst_check in which tp->dst can be released.
>
> The problem is before calling sctp_transport_dst_check, it doesn't
> check sock_owned_by_user, which means tp->dst could be freed while
> a process is accessing it with owning the socket.
>
> An use-after-free issue could be triggered by this.
>
> This patch is to fix it by checking sock_owned_by_user before calling
> sctp_transport_dst_check in sctp_icmp_redirect, so that it would not
> release tp->dst if users still hold sock lock.
>
> Besides, the same issue fixed in commit 45caeaa5ac0b ("dccp/tcp: fix
> routing redirect race") on sctp also needs this check.
>
> Fixes: 55be7a9c6074 ("ipv4: Add redirect support to all protocol icmp error handlers")
> Reported-by: Eric Dumazet <edumazet@google.com>
Nice catch, btw.
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Thanks
> ---
> net/sctp/input.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 92a0714..34f10e7 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -421,7 +421,7 @@ void sctp_icmp_redirect(struct sock *sk, struct sctp_transport *t,
> {
> struct dst_entry *dst;
>
> - if (!t)
> + if (sock_owned_by_user(sk) || !t)
> return;
> dst = sctp_transport_dst_check(t);
> if (dst)
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH 1/7] devlink: Add permanent config parameter get/set operations
From: Jiri Pirko @ 2017-10-18 14:01 UTC (permalink / raw)
To: Steve Lin
Cc: netdev, Jiri Pirko, David S . Miller, Michael Chan, John Linville,
Andy Gospodarek
In-Reply-To: <CA+Jmh7Etbr_E-E35fY79s4ss+nsgH4+wN=RQiK4WCcP-XXe1Vw@mail.gmail.com>
Wed, Oct 18, 2017 at 03:14:35PM CEST, steven.lin1@broadcom.com wrote:
>On Wed, Oct 18, 2017 at 8:58 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Oct 18, 2017 at 02:39:09PM CEST, steven.lin1@broadcom.com wrote:
>>>On Wed, Oct 18, 2017 at 3:11 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Tue, Oct 17, 2017 at 10:44:23PM CEST, steven.lin1@broadcom.com wrote:
>>>> Steve. As I originally requested, could you please split this to:
>>>> 1) single patch adding config get/set commands, without any config attributes
>>>> 2) single patch per config attribute - please don't add them in bulk.
>>>> We also need very strict description for every single attribute so
>>>> other vendors know what it is and can re-use it. There is need to
>>>> avoid duplication here. Also, please send just few attribites in the
>>>> first run, not like 40 you are sending now. Impossible to review.
>>>
>>>I broke the patch set up into functional blocks of attributes, in
>>>order to avoid having ~40 patches of just a couple lines each. But, I
>>>will split further for each individual attribute, and just submit a
>>>few initially, per your request.
>>>
>>>>
>>>> Also, why didn't you put it into nested attribute we were discussing?
>>>>
>>>
>>>I thought I did :) , using the DPIPE_HEADERS nested attribute as an
>>>example. I'll reach out to you off-list to understand what I'm
>>>missing.
>>
>> I missed that. But you need a separate attr enum as well.
>>
>
>I did have this as the nested attr enum in the original patch:
No, I mean separate "enum your_new_enum"
>
> /* Permanent Configuration Parameters */
> DEVLINK_ATTR_PERM_CFG, /* nested */
>
>However, I only used the nested construct in the response from kernel
>to userspace, not in the request from userspace to kernel. (This was
>based on looking at the various DPIPE_* nested attributes as
>examples.)
>
>Thinking about it after seeing your comment, I'm thinking I should
>also use the nested attribute construct in the original request from
>userspace to kernel as well, although I didn't see any previous
>examples of this in devlink.
>
>So I'll plan to use nesting in that direction as well.
>
>Thanks,
>Steve
^ permalink raw reply
* Re: [PATCH net 0/3] Fix for BPF devmap percpu allocation splat
From: Daniel Borkmann @ 2017-10-18 14:03 UTC (permalink / raw)
To: Tejun Heo
Cc: davem, ast, john.fastabend, mark.rutland, richard, sp3485, netdev,
linux-kernel, Dennis Zhou
In-Reply-To: <20171018132526.GC1302522@devbig577.frc2.facebook.com>
On 10/18/2017 03:25 PM, Tejun Heo wrote:
> Hello, Daniel.
>
> (cc'ing Dennis)
>
> On Tue, Oct 17, 2017 at 04:55:51PM +0200, Daniel Borkmann wrote:
>> The set fixes a splat in devmap percpu allocation when we alloc
>> the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
>> patch 1 is rather small, so if this could be routed via -net, for
>> example, with Tejun's Ack that would be good. Patch 3 gets rid of
>> remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
>> internals and should not be used.
>>
>> Thanks!
>>
>> Daniel Borkmann (3):
>> mm, percpu: add support for __GFP_NOWARN flag
>
> This looks fine.
Great, thanks!
>> bpf: fix splat for illegal devmap percpu allocation
>> bpf: do not test for PCPU_MIN_UNIT_SIZE before percpu allocations
>
> These look okay too but if it helps percpu allocator can expose the
> maximum size / alignment supported to take out the guessing game too.
At least from BPF side there's right now no infra for exposing
max possible alloc sizes for maps to e.g. user space as indication.
There are few users left in the tree, where it would make sense for
having some helpers though:
arch/tile/kernel/setup.c:729: if (size < PCPU_MIN_UNIT_SIZE)
arch/tile/kernel/setup.c:730: size = PCPU_MIN_UNIT_SIZE;
drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:346: unsigned int max = (PCPU_MIN_UNIT_SIZE - sizeof(*pools)) << 3;
drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:352: /* make sure per cpu pool fits into PCPU_MIN_UNIT_SIZE */
drivers/scsi/libfc/fc_exch.c:2488: /* reduce range so per cpu pool fits into PCPU_MIN_UNIT_SIZE pool */
drivers/scsi/libfc/fc_exch.c:2489: pool_exch_range = (PCPU_MIN_UNIT_SIZE - sizeof(*pool)) /
> Also, the reason why PCPU_MIN_UNIT_SIZE is what it is is because
> nobody needed anything bigger. Increasing the size doesn't really
> cost much at least on 64bit archs. Is that something we want to be
> considering?
For devmap (and cpumap) itself it wouldn't make sense. For per-cpu
hashtable we could indeed consider it in the future.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH 4/7] devlink: Adding perm config of link settings
From: Jiri Pirko @ 2017-10-18 14:04 UTC (permalink / raw)
To: Steve Lin
Cc: netdev, Jiri Pirko, David S . Miller, Michael Chan, John Linville,
Andy Gospodarek
In-Reply-To: <CA+Jmh7HnZG+7xP=02K3BMKxODbfP=Mq=9o5iC2fS4LXw0+djMg@mail.gmail.com>
Wed, Oct 18, 2017 at 03:22:03PM CEST, steven.lin1@broadcom.com wrote:
>On Wed, Oct 18, 2017 at 9:01 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Oct 18, 2017 at 02:39:42PM CEST, steven.lin1@broadcom.com wrote:
>>>On Wed, Oct 18, 2017 at 3:31 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>
>>>> You need to split the config option to those that are per-port and to
>>>> those that are per-asic. For each family, you have to use ither
>>>> devlink_port of devlink handle. Also, you need to split into those that are
>>>> permanent and to those who are teporary (until reset). I think you might
>>>> need some flags for that.
>>>>
>>>
>>>All these are permanent; none are temporary - that's (partially) why
>>>we consider these to be devlink/device parameters rather than a
>>>netdev/ethtool thing, since they take effect after the next reset and
>>>before any drivers are loaded (i.e. the card uses these parameters for
>>>its default/startup configuration).
>>
>> Understood. But I think that this iface should be capable to serve the
>> options of non-permanent as well. Or this could be 2 separate interfaces
>> with 2 separate cmd pair. Thoughts?
>>
>
>I would prefer to keep this command for permanent config only, and use
>a separate command for transient configuration. I think that
>transient device configuration should be tackled in the separate
>discussion that was started in the RFC version of this patchset,
>related to moving ethtool ops to devlink/netlink.
>
>I think that's a separate topic that requires a little more thought
>and discussion, but really isn't that related to this current patchset
>for permanent device configuration. What do you think?
Makes sense. Then you need to clearly mark the cmds with "permanent"
keyword in name and enhance the description.
^ permalink raw reply
* Re: [iproute2] regression in ss output
From: Humberto Alves @ 2017-10-18 14:08 UTC (permalink / raw)
To: Stephen Hemminger, Eric Dumazet; +Cc: Phil Sutter, netdev@vger.kernel.org
In-Reply-To: <20171016194958.669625db@xeon-e3>
[-- Attachment #1: Type: text/plain, Size: 855 bytes --]
Looks perfect to me. Thanks!
On 10/17/2017 03:49 AM, Stephen Hemminger wrote:
> On Mon, 16 Oct 2017 19:00:36 -0700
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>> On Mon, 2017-10-16 at 14:28 -0700, Stephen Hemminger wrote:
>>> On Mon, 16 Oct 2017 20:44:07 +0000
>>> Humberto Alves <hjalves@live.com> wrote:
>>>
>>>> Yes, just get rid of this 'if statement'.
>>>> in6addr_any should be represented as '::', not '*'. Otherwise it's
>>>> impossible to distinguish IPv4 listening addresses from IPv6. Thank you :)
>>>
>>> But IPv6 accepts IPv4 as well.
>>
>> Not always (IPV6_V6ONLY socket option)
>>
>> I agree that this recent change in ss is problematic.
>>
>> Please give us back a way to distinguish IPV6 and IPv4
>
> What about
> *:80 << both IPV6 and IPV4
> [::]:80 << IPV6_ONLY
> 0.0.0.0:80 << IPV4_ONLY
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3984 bytes --]
^ permalink raw reply
* [net PATCH 0/5] sockmap fixes for net
From: John Fastabend @ 2017-10-18 14:09 UTC (permalink / raw)
To: alexei.starovoitov, davem; +Cc: netdev, borkmann
The following implements a set of fixes for sockmap and changes the
API slightly in a few places to reduce preempt_disable/enable scope.
We do this here in net because it requires an API change and this
avoids getting stuck with legacy API going forward.
The short description:
Access to skb mark is removed, it is problematic when we add
features in the future because mark is a union and used by the
TCP/socket code internally. We don't want to expose this to the
BPF programs or let programs change the values.
The other change is caching metadata in the skb itself between
when the BPF program returns a redirect code and the core code
implements the redirect. This avoids having per cpu metadata.
Finally, tighten restriction on using sockmap to CAP_NET_ADMIN and
only SOCK_STREAM sockets.
Thanks,
John
---
John Fastabend (5):
bpf: enforce TCP only support for sockmap
bpf: avoid preempt enable/disable in sockmap using tcp_skb_cb region
bpf: remove mark access for SK_SKB program types
bpf: require CAP_NET_ADMIN when using sockmap maps
bpf: require CAP_NET_ADMIN when using devmap
include/linux/filter.h | 2 +
include/net/tcp.h | 5 +++
kernel/bpf/devmap.c | 3 ++
kernel/bpf/sockmap.c | 28 ++++++++++++------
net/core/filter.c | 31 ++++++++++----------
samples/sockmap/sockmap_kern.c | 2 +
tools/include/uapi/linux/bpf.h | 3 +-
tools/testing/selftests/bpf/bpf_helpers.h | 2 +
tools/testing/selftests/bpf/sockmap_verdict_prog.c | 4 +--
tools/testing/selftests/bpf/test_maps.c | 12 +++++++-
tools/testing/selftests/bpf/test_verifier.c | 16 +++++++++-
11 files changed, 74 insertions(+), 34 deletions(-)
^ permalink raw reply
* [net PATCH 1/5] bpf: enforce TCP only support for sockmap
From: John Fastabend @ 2017-10-18 14:10 UTC (permalink / raw)
To: alexei.starovoitov, davem; +Cc: netdev, borkmann
In-Reply-To: <150833522977.3588.14633565129152334098.stgit@john-XPS-13-9360>
From: John Fastabend <john.fastabend@gmail.com>
Only TCP sockets have been tested and at the moment the state change
callback only handles TCP sockets. This adds a check to ensure that
sockets actually being added are TCP sockets.
For net-next we can consider UDP support.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/sockmap.c | 6 ++++++
tools/testing/selftests/bpf/test_maps.c | 12 +++++++++++-
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 6424ce0..c68899d 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -840,6 +840,12 @@ static int sock_map_update_elem(struct bpf_map *map,
return -EINVAL;
}
+ if (skops.sk->sk_type != SOCK_STREAM ||
+ skops.sk->sk_protocol != IPPROTO_TCP) {
+ fput(socket->file);
+ return -EOPNOTSUPP;
+ }
+
err = sock_map_ctx_update_elem(&skops, map, key, flags);
fput(socket->file);
return err;
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index fe3a443..50ce52d 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -466,7 +466,7 @@ static void test_sockmap(int tasks, void *data)
int one = 1, map_fd_rx, map_fd_tx, map_fd_break, s, sc, rc;
struct bpf_map *bpf_map_rx, *bpf_map_tx, *bpf_map_break;
int ports[] = {50200, 50201, 50202, 50204};
- int err, i, fd, sfd[6] = {0xdeadbeef};
+ int err, i, fd, udp, sfd[6] = {0xdeadbeef};
u8 buf[20] = {0x0, 0x5, 0x3, 0x2, 0x1, 0x0};
int parse_prog, verdict_prog;
struct sockaddr_in addr;
@@ -548,6 +548,16 @@ static void test_sockmap(int tasks, void *data)
goto out_sockmap;
}
+ /* Test update with unsupported UDP socket */
+ udp = socket(AF_INET, SOCK_DGRAM, 0);
+ i = 0;
+ err = bpf_map_update_elem(fd, &i, &udp, BPF_ANY);
+ if (!err) {
+ printf("Failed socket SOCK_DGRAM allowed '%i:%i'\n",
+ i, udp);
+ goto out_sockmap;
+ }
+
/* Test update without programs */
for (i = 0; i < 6; i++) {
err = bpf_map_update_elem(fd, &i, &sfd[i], BPF_ANY);
^ permalink raw reply related
* [net PATCH 2/5] bpf: avoid preempt enable/disable in sockmap using tcp_skb_cb region
From: John Fastabend @ 2017-10-18 14:10 UTC (permalink / raw)
To: alexei.starovoitov, davem; +Cc: netdev, borkmann
In-Reply-To: <150833522977.3588.14633565129152334098.stgit@john-XPS-13-9360>
From: John Fastabend <john.fastabend@gmail.com>
SK_SKB BPF programs are run from the socket/tcp context but early in
the stack before much of the TCP metadata is needed in tcp_skb_cb. So
we can use some unused fields to place BPF metadata needed for SK_SKB
programs when implementing the redirect function.
This allows us to drop the preempt disable logic. It does however
require an API change so sk_redirect_map() has been updated to
additionally provide ctx_ptr to skb. Note, we do however continue to
disable/enable preemption around actual BPF program running to account
for map updates.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
include/linux/filter.h | 2 +
include/net/tcp.h | 5 +++
kernel/bpf/sockmap.c | 19 ++++++-------
net/core/filter.c | 29 ++++++++++----------
samples/sockmap/sockmap_kern.c | 2 +
tools/include/uapi/linux/bpf.h | 3 +-
tools/testing/selftests/bpf/bpf_helpers.h | 2 +
tools/testing/selftests/bpf/sockmap_verdict_prog.c | 4 +--
8 files changed, 36 insertions(+), 30 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index d29e58f..818a0b2 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -728,7 +728,7 @@ void xdp_do_flush_map(void);
void bpf_warn_invalid_xdp_action(u32 act);
void bpf_warn_invalid_xdp_redirect(u32 ifindex);
-struct sock *do_sk_redirect_map(void);
+struct sock *do_sk_redirect_map(struct sk_buff *skb);
#ifdef CONFIG_BPF_JIT
extern int bpf_jit_enable;
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 89974c5..b1ef98e 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -840,6 +840,11 @@ struct tcp_skb_cb {
struct inet6_skb_parm h6;
#endif
} header; /* For incoming skbs */
+ struct {
+ __u32 key;
+ __u32 flags;
+ struct bpf_map *map;
+ } bpf;
};
};
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index c68899d..beaabb2 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -39,6 +39,7 @@
#include <linux/workqueue.h>
#include <linux/list.h>
#include <net/strparser.h>
+#include <net/tcp.h>
struct bpf_stab {
struct bpf_map map;
@@ -101,9 +102,16 @@ static int smap_verdict_func(struct smap_psock *psock, struct sk_buff *skb)
return SK_DROP;
skb_orphan(skb);
+ /* We need to ensure that BPF metadata for maps is also cleared
+ * when we orphan the skb so that we don't have the possibility
+ * to reference a stale map.
+ */
+ TCP_SKB_CB(skb)->bpf.map = NULL;
skb->sk = psock->sock;
bpf_compute_data_end(skb);
+ preempt_disable();
rc = (*prog->bpf_func)(skb, prog->insnsi);
+ preempt_enable();
skb->sk = NULL;
return rc;
@@ -114,17 +122,10 @@ static void smap_do_verdict(struct smap_psock *psock, struct sk_buff *skb)
struct sock *sk;
int rc;
- /* Because we use per cpu values to feed input from sock redirect
- * in BPF program to do_sk_redirect_map() call we need to ensure we
- * are not preempted. RCU read lock is not sufficient in this case
- * with CONFIG_PREEMPT_RCU enabled so we must be explicit here.
- */
- preempt_disable();
rc = smap_verdict_func(psock, skb);
switch (rc) {
case SK_REDIRECT:
- sk = do_sk_redirect_map();
- preempt_enable();
+ sk = do_sk_redirect_map(skb);
if (likely(sk)) {
struct smap_psock *peer = smap_psock_sk(sk);
@@ -141,8 +142,6 @@ static void smap_do_verdict(struct smap_psock *psock, struct sk_buff *skb)
/* Fall through and free skb otherwise */
case SK_DROP:
default:
- if (rc != SK_REDIRECT)
- preempt_enable();
kfree_skb(skb);
}
}
diff --git a/net/core/filter.c b/net/core/filter.c
index 74b8c91..ca1ba0b 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1839,31 +1839,31 @@ static const struct bpf_func_proto bpf_redirect_proto = {
.arg2_type = ARG_ANYTHING,
};
-BPF_CALL_3(bpf_sk_redirect_map, struct bpf_map *, map, u32, key, u64, flags)
+BPF_CALL_4(bpf_sk_redirect_map, struct sk_buff *, skb,
+ struct bpf_map *, map, u32, key, u64, flags)
{
- struct redirect_info *ri = this_cpu_ptr(&redirect_info);
+ struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
if (unlikely(flags))
return SK_ABORTED;
- ri->ifindex = key;
- ri->flags = flags;
- ri->map = map;
+ tcb->bpf.key = key;
+ tcb->bpf.flags = flags;
+ tcb->bpf.map = map;
return SK_REDIRECT;
}
-struct sock *do_sk_redirect_map(void)
+struct sock *do_sk_redirect_map(struct sk_buff *skb)
{
- struct redirect_info *ri = this_cpu_ptr(&redirect_info);
+ struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
struct sock *sk = NULL;
- if (ri->map) {
- sk = __sock_map_lookup_elem(ri->map, ri->ifindex);
+ if (tcb->bpf.map) {
+ sk = __sock_map_lookup_elem(tcb->bpf.map, tcb->bpf.key);
- ri->ifindex = 0;
- ri->map = NULL;
- /* we do not clear flags for future lookup */
+ tcb->bpf.key = 0;
+ tcb->bpf.map = NULL;
}
return sk;
@@ -1873,9 +1873,10 @@ static const struct bpf_func_proto bpf_sk_redirect_map_proto = {
.func = bpf_sk_redirect_map,
.gpl_only = false,
.ret_type = RET_INTEGER,
- .arg1_type = ARG_CONST_MAP_PTR,
- .arg2_type = ARG_ANYTHING,
+ .arg1_type = ARG_PTR_TO_CTX,
+ .arg2_type = ARG_CONST_MAP_PTR,
.arg3_type = ARG_ANYTHING,
+ .arg4_type = ARG_ANYTHING,
};
BPF_CALL_1(bpf_get_cgroup_classid, const struct sk_buff *, skb)
diff --git a/samples/sockmap/sockmap_kern.c b/samples/sockmap/sockmap_kern.c
index f9b38ef..52b0053 100644
--- a/samples/sockmap/sockmap_kern.c
+++ b/samples/sockmap/sockmap_kern.c
@@ -62,7 +62,7 @@ int bpf_prog2(struct __sk_buff *skb)
ret = 1;
bpf_printk("sockmap: %d -> %d @ %d\n", lport, bpf_ntohl(rport), ret);
- return bpf_sk_redirect_map(&sock_map, ret, 0);
+ return bpf_sk_redirect_map(skb, &sock_map, ret, 0);
}
SEC("sockops")
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 43ab5c4..be9a631 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -569,9 +569,10 @@ union bpf_attr {
* @flags: reserved for future use
* Return: 0 on success or negative error code
*
- * int bpf_sk_redirect_map(map, key, flags)
+ * int bpf_sk_redirect_map(skb, map, key, flags)
* Redirect skb to a sock in map using key as a lookup key for the
* sock in map.
+ * @skb: pointer to skb
* @map: pointer to sockmap
* @key: key to lookup sock in map
* @flags: reserved for future use
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index 36fb916..b2e02bd 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -65,7 +65,7 @@ static int (*bpf_xdp_adjust_head)(void *ctx, int offset) =
static int (*bpf_setsockopt)(void *ctx, int level, int optname, void *optval,
int optlen) =
(void *) BPF_FUNC_setsockopt;
-static int (*bpf_sk_redirect_map)(void *map, int key, int flags) =
+static int (*bpf_sk_redirect_map)(void *ctx, void *map, int key, int flags) =
(void *) BPF_FUNC_sk_redirect_map;
static int (*bpf_sock_map_update)(void *map, void *key, void *value,
unsigned long long flags) =
diff --git a/tools/testing/selftests/bpf/sockmap_verdict_prog.c b/tools/testing/selftests/bpf/sockmap_verdict_prog.c
index 9b99bd1..2cd2d55 100644
--- a/tools/testing/selftests/bpf/sockmap_verdict_prog.c
+++ b/tools/testing/selftests/bpf/sockmap_verdict_prog.c
@@ -61,8 +61,8 @@ int bpf_prog2(struct __sk_buff *skb)
bpf_printk("verdict: data[0] = redir(%u:%u)\n", map, sk);
if (!map)
- return bpf_sk_redirect_map(&sock_map_rx, sk, 0);
- return bpf_sk_redirect_map(&sock_map_tx, sk, 0);
+ return bpf_sk_redirect_map(skb, &sock_map_rx, sk, 0);
+ return bpf_sk_redirect_map(skb, &sock_map_tx, sk, 0);
}
char _license[] SEC("license") = "GPL";
^ permalink raw reply related
* [net PATCH 3/5] bpf: remove mark access for SK_SKB program types
From: John Fastabend @ 2017-10-18 14:10 UTC (permalink / raw)
To: alexei.starovoitov, davem; +Cc: netdev, borkmann
In-Reply-To: <150833522977.3588.14633565129152334098.stgit@john-XPS-13-9360>
From: John Fastabend <john.fastabend@gmail.com>
The skb->mark field is a union with reserved_tailroom which is used
in the TCP code paths from stream memory allocation. Allowing SK_SKB
programs to set this field creates a conflict with future code
optimizations, such as "gifting" the skb to the egress path instead
of creating a new skb and doing a memcpy.
Because we do not have a released version of SK_SKB yet lets just
remove it for now. A more appropriate scratch pad to use at the
socket layer is dev_scratch, but lets add that in future kernels
when needed.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/filter.c | 2 +-
tools/testing/selftests/bpf/test_verifier.c | 16 ++++++++++++++--
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index ca1ba0b..aa02659 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3684,7 +3684,6 @@ static bool sk_skb_is_valid_access(int off, int size,
{
if (type == BPF_WRITE) {
switch (off) {
- case bpf_ctx_range(struct __sk_buff, mark):
case bpf_ctx_range(struct __sk_buff, tc_index):
case bpf_ctx_range(struct __sk_buff, priority):
break;
@@ -3694,6 +3693,7 @@ static bool sk_skb_is_valid_access(int off, int size,
}
switch (off) {
+ case bpf_ctx_range(struct __sk_buff, mark):
case bpf_ctx_range(struct __sk_buff, tc_classid):
return false;
case bpf_ctx_range(struct __sk_buff, data):
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 26f3250..16cca57 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -1130,15 +1130,27 @@ static struct bpf_test tests[] = {
.errstr = "invalid bpf_context access",
},
{
- "check skb->mark is writeable by SK_SKB",
+ "invalid access of skb->mark for SK_SKB",
+ .insns = {
+ BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_1,
+ offsetof(struct __sk_buff, mark)),
+ BPF_EXIT_INSN(),
+ },
+ .result = REJECT,
+ .prog_type = BPF_PROG_TYPE_SK_SKB,
+ .errstr = "invalid bpf_context access",
+ },
+ {
+ "check skb->mark is not writeable by SK_SKB",
.insns = {
BPF_MOV64_IMM(BPF_REG_0, 0),
BPF_STX_MEM(BPF_W, BPF_REG_1, BPF_REG_0,
offsetof(struct __sk_buff, mark)),
BPF_EXIT_INSN(),
},
- .result = ACCEPT,
+ .result = REJECT,
.prog_type = BPF_PROG_TYPE_SK_SKB,
+ .errstr = "invalid bpf_context access",
},
{
"check skb->tc_index is writeable by SK_SKB",
^ permalink raw reply related
* [net PATCH 4/5] bpf: require CAP_NET_ADMIN when using sockmap maps
From: John Fastabend @ 2017-10-18 14:11 UTC (permalink / raw)
To: alexei.starovoitov, davem; +Cc: netdev, borkmann
In-Reply-To: <150833522977.3588.14633565129152334098.stgit@john-XPS-13-9360>
From: John Fastabend <john.fastabend@gmail.com>
Restrict sockmap to CAP_NET_ADMIN.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/sockmap.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index beaabb2..2b6eb35 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -486,6 +486,9 @@ static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
int err = -EINVAL;
u64 cost;
+ if (!capable(CAP_NET_ADMIN))
+ return ERR_PTR(-EPERM);
+
/* check sanity of attributes */
if (attr->max_entries == 0 || attr->key_size != 4 ||
attr->value_size != 4 || attr->map_flags & ~BPF_F_NUMA_NODE)
^ permalink raw reply related
* [net PATCH 5/5] bpf: require CAP_NET_ADMIN when using devmap
From: John Fastabend @ 2017-10-18 14:11 UTC (permalink / raw)
To: alexei.starovoitov, davem; +Cc: netdev, borkmann
In-Reply-To: <150833522977.3588.14633565129152334098.stgit@john-XPS-13-9360>
From: John Fastabend <john.fastabend@gmail.com>
Devmap is used with XDP which requires CAP_NET_ADMIN so lets also
make CAP_NET_ADMIN required to use the map.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
kernel/bpf/devmap.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index e093d9a..7d9f32f 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -78,6 +78,9 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
int err = -EINVAL;
u64 cost;
+ if (!capable(CAP_NET_ADMIN))
+ return ERR_PTR(-EPERM);
+
/* check sanity of attributes */
if (attr->max_entries == 0 || attr->key_size != 4 ||
attr->value_size != 4 || attr->map_flags & ~BPF_F_NUMA_NODE)
^ permalink raw reply related
* Re: [PATCH] staging: irda: resolve sparse errors due to implicit pci_power_t casts
From: Greg KH @ 2017-10-18 14:12 UTC (permalink / raw)
To: Matthew Giassa
Cc: samuel, devel, georgiana.chelu93, johannes.berg, netdev,
linux-kernel, davem, khoroshilov
In-Reply-To: <1507246703-1736-1-git-send-email-matthew@giassa.net>
On Thu, Oct 05, 2017 at 04:38:23PM -0700, Matthew Giassa wrote:
> Explicitly casting pci_power_t types to resolve sparse warnings (shown
> below).
>
> Also fixing a related logging bug where pci_power_t is cast to unsigned
> (can be negative, i.e. PCI_POWER_ERROR).
>
> Original sparse report:
>
> drivers/staging/irda/drivers//vlsi_ir.c:170:51: warning: cast from
> restricted pci_power_t
> drivers/staging/irda/drivers//vlsi_ir.c:1726:39: warning: restricted
> pci_power_t degrades to integer
> drivers/staging/irda/drivers//vlsi_ir.c:1728:45: warning: incorrect type
> in assignment (different base types)
> drivers/staging/irda/drivers//vlsi_ir.c:1728:45: expected restricted
> pci_power_t [usertype] current_state
> drivers/staging/irda/drivers//vlsi_ir.c:1728:45: got int [signed]
> [usertype] event
> drivers/staging/irda/drivers//vlsi_ir.c:1748:29: warning: incorrect type
> in assignment (different base types)
> drivers/staging/irda/drivers//vlsi_ir.c:1748:29: expected restricted
> pci_power_t [usertype] current_state
> drivers/staging/irda/drivers//vlsi_ir.c:1748:29: got int [signed]
> [usertype] event
Please do not line-wrap lines like this, it makes them harder to
understand.
>
> Warnings no longer present.
>
> Signed-off-by: Matthew Giassa <matthew@giassa.net>
> ---
> drivers/staging/irda/drivers/vlsi_ir.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/irda/drivers/vlsi_ir.c b/drivers/staging/irda/drivers/vlsi_ir.c
> index 3dff3c5..20ce4d8 100644
> --- a/drivers/staging/irda/drivers/vlsi_ir.c
> +++ b/drivers/staging/irda/drivers/vlsi_ir.c
> @@ -167,7 +167,8 @@ static void vlsi_proc_pdev(struct seq_file *seq, struct pci_dev *pdev)
>
> seq_printf(seq, "\n%s (vid/did: [%04x:%04x])\n",
> pci_name(pdev), (int)pdev->vendor, (int)pdev->device);
> - seq_printf(seq, "pci-power-state: %u\n", (unsigned) pdev->current_state);
> + seq_printf(seq, "pci-power-state: %d\n",
> + (int __force)pdev->current_state);
Ick, using __force is almost always a huge sign that something is wrong
here. This patch does not look correct because of this.
You did read drivers/staging/irda/TODO, right?
thanks,
greg k-h
^ permalink raw reply
* Re: [net-next V8 PATCH 3/5] bpf: cpumap xdp_buff to skb conversion and allocation
From: Michael S. Tsirkin @ 2017-10-18 14:12 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, jakub.kicinski, pavel.odintsov, Jason Wang, mchan,
John Fastabend, peter.waskiewicz.jr, ast, Daniel Borkmann,
Alexei Starovoitov, Andy Gospodarek
In-Reply-To: <150814917905.1806.2745716159653488848.stgit@firesoul>
On Mon, Oct 16, 2017 at 12:19:39PM +0200, Jesper Dangaard Brouer wrote:
> @@ -191,15 +280,45 @@ static int cpu_map_kthread_run(void *data)
> * kthread_stop signal until queue is empty.
> */
> while (!kthread_should_stop() || !__ptr_ring_empty(rcpu->queue)) {
> + unsigned int processed = 0, drops = 0;
> struct xdp_pkt *xdp_pkt;
>
> - schedule();
> - /* Do work */
> - while ((xdp_pkt = ptr_ring_consume(rcpu->queue))) {
> - /* For now just "refcnt-free" */
> - page_frag_free(xdp_pkt);
> + /* Release CPU reschedule checks */
> + if (__ptr_ring_empty(rcpu->queue)) {
I suspect this is racy: if ring becomes non empty here and
you wake the task, next line will put it to sleep.
I think you want to reverse the order:
__set_current_state(TASK_INTERRUPTIBLE);
then check __ptr_ring_empty.
I note using the __ version means you can not resize the ring.
Hope you do not need to.
> + __set_current_state(TASK_INTERRUPTIBLE);
> + schedule();
> + } else {
> + cond_resched();
> + }
> + __set_current_state(TASK_RUNNING);
> +
> + /* Process packets in rcpu->queue */
> + local_bh_disable();
> + /*
> + * The bpf_cpu_map_entry is single consumer, with this
> + * kthread CPU pinned. Lockless access to ptr_ring
> + * consume side valid as no-resize allowed of queue.
> + */
> + while ((xdp_pkt = __ptr_ring_consume(rcpu->queue))) {
> + struct sk_buff *skb;
> + int ret;
> +
> + skb = cpu_map_build_skb(rcpu, xdp_pkt);
> + if (!skb) {
> + page_frag_free(xdp_pkt);
> + continue;
> + }
> +
> + /* Inject into network stack */
> + ret = netif_receive_skb_core(skb);
> + if (ret == NET_RX_DROP)
> + drops++;
> +
> + /* Limit BH-disable period */
> + if (++processed == 8)
> + break;
> }
> - __set_current_state(TASK_INTERRUPTIBLE);
> + local_bh_enable(); /* resched point, may call do_softirq() */
> }
> __set_current_state(TASK_RUNNING);
>
> @@ -490,13 +609,6 @@ static int bq_flush_to_queue(struct bpf_cpu_map_entry *rcpu,
> return 0;
> }
>
> -/* Notice: Will change in later patch */
> -struct xdp_pkt {
> - void *data;
> - u16 len;
> - u16 headroom;
> -};
> -
> /* Runs under RCU-read-side, plus in softirq under NAPI protection.
> * Thus, safe percpu variable access.
> */
> @@ -524,17 +636,13 @@ int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp,
> struct net_device *dev_rx)
> {
> struct xdp_pkt *xdp_pkt;
> - int headroom;
>
> - /* For now this is just used as a void pointer to data_hard_start.
> - * Followup patch will generalize this.
> - */
> - xdp_pkt = xdp->data_hard_start;
> + xdp_pkt = convert_to_xdp_pkt(xdp);
> + if (!xdp_pkt)
> + return -EOVERFLOW;
>
> - /* Fake writing into xdp_pkt->data to measure overhead */
> - headroom = xdp->data - xdp->data_hard_start;
> - if (headroom < sizeof(*xdp_pkt))
> - xdp_pkt->data = xdp->data;
> + /* Info needed when constructing SKB on remote CPU */
> + xdp_pkt->dev_rx = dev_rx;
>
> bq_enqueue(rcpu, xdp_pkt);
> return 0;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fcddccb6be41..36bb68f3a2c7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4491,6 +4491,33 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> return ret;
> }
>
> +/**
> + * netif_receive_skb_core - special purpose version of netif_receive_skb
> + * @skb: buffer to process
> + *
> + * More direct receive version of netif_receive_skb(). It should
> + * only be used by callers that have a need to skip RPS and Generic XDP.
> + * Caller must also take care of handling if (page_is_)pfmemalloc.
> + *
> + * This function may only be called from softirq context and interrupts
> + * should be enabled.
> + *
> + * Return values (usually ignored):
> + * NET_RX_SUCCESS: no congestion
> + * NET_RX_DROP: packet was dropped
> + */
> +int netif_receive_skb_core(struct sk_buff *skb)
> +{
> + int ret;
> +
> + rcu_read_lock();
> + ret = __netif_receive_skb_core(skb, false);
> + rcu_read_unlock();
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(netif_receive_skb_core);
> +
> static int __netif_receive_skb(struct sk_buff *skb)
> {
> int ret;
^ permalink raw reply
* Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates
From: Franklin S Cooper Jr @ 2017-10-18 14:17 UTC (permalink / raw)
To: Sekhar Nori, Marc Kleine-Budde, Mario Hüttel, Yang, Wenyou,
wg, socketcan, quentin.schulz, edumazet, linux-can, netdev,
linux-kernel
Cc: Wenyou Yang, Dong Aisheng, Quadros, Roger
In-Reply-To: <7ba515d9-7710-c152-a55a-f995b7f3d49a@ti.com>
On 10/18/2017 08:24 AM, Sekhar Nori wrote:
> Hi Marc,
>
> On Wednesday 18 October 2017 06:14 PM, Marc Kleine-Budde wrote:
>> On 09/21/2017 02:48 AM, Franklin S Cooper Jr wrote:
>>>
>>>
>>> On 09/20/2017 04:37 PM, Mario Hüttel wrote:
>>>>
>>>>
>>>> On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote:
>>>>> Hi Wenyou,
>>>>>
>>>>> On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>>>>>>
>>>>>> On 2017/9/14 13:06, Sekhar Nori wrote:
>>>>>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>>>>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>>>>>>> bit
>>>>>>>>> was being transmitted and with a bit more investigation realized the
>>>>>>>>> actual
>>>>>>>>> MCAN IP would go back to initialization mode automatically.
>>>>>>>>>
>>>>>>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>>>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>>>>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>>>>>>> Compensation Offset register should be set. The document mentions
>>>>>>>>> that this
>>>>>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>>>>>>
>>>>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>>>>>>> indicates
>>>>>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>>>>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>>>>>>> rate
>>>>>>>>> is above 2.5 Mbps.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>>>>>> ---
>>>>>>>>> I'm pretty surprised that this hasn't been implemented already since
>>>>>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>>>>>>> supports up to 10 Mbps.
>>>>>>>>>
>>>>>>>>> So it will be nice to get comments from users of this driver to
>>>>>>>>> understand
>>>>>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>>>>>>> patch.
>>>>>>>>> If they haven't what did they do to get around it if they needed higher
>>>>>>>>> speeds.
>>>>>>>>>
>>>>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>>>>>>> insure
>>>>>>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>>>>>>> transceiver.
>>>>>>>> ping. Anyone has any thoughts on this?
>>>>>>> I added Dong who authored the m_can driver and Wenyou who added the only
>>>>>>> in-kernel user of the driver for any help.
>>>>>> I tested it on SAMA5D2 Xplained board both with and without this patch,
>>>>>> both work with the 4M bps data bit rate.
>>>>> Thank you for testing this out. Its interesting that you have been able
>>>>> to use higher speeds without this patch. What is the CAN transceiver
>>>>> being used on the SAMA5D2 Xplained board? I tried looking at the
>>>>> schematic but it seems the CAN signals are used on an extension board
>>>>> which I can't find the schematic for. Also do you mind sharing your test
>>>>> setup? Were you doing a short point to point test?
>>>>>
>>>>> Thank You,
>>>>> Franklin
>>>> Hello Franklin,
>>>>
>>>> your patch definitely makes sense.
>>>>
>>>> I forgot the TDC in my patches because it was not present in the
>>>> previous driver versions and because I didn't encounter any
>>>> problems when testing it myself.
>>>>
>>>> The error is highly dependent on the hardware (transceiver) setup.
>>>> So it is definitely possible that some people don't encounter errors
>>>> without your patch.
>>>
>>> So the Transmission Delay Compensation feature Value register is suppose
>>> to take into consideration the transceiver delay automatically and add
>>> the value of TDCO on top of that. So why would TDCO be dependent on the
>>> transceiver? I've heard conflicting things regarding TDC so any
>>> clarification on what actually impacts it would be appreciated.
>>>
>>> Also part of the issue I'm having is how can we properly configure TDCO?
>>> Configuring TDCO is essentially figuring out what Secondary Sample Point
>>> to use. However, it is unclear what value to set SSP to and which use
>>> cases a given SSP will work or doesn't work. I've seen various
>>> recommendations from Bosch on choosing SSP but ultimately it seems they
>>> suggestion "real world testing" to come up with a proper value. Not
>>> setting TDCO causes problems for my device and improperly setting TDCO
>>> causes problems for my device. So its likely any value I use could end
>>> up breaking something for someone else.
>>>
>>> Currently I leaning to a DT property that can be used for setting SSP.
>>> Perhaps use a generic default value and allow individuals to override it
>>> via DT?
>>
>> Sounds reasonable. What's the status of this series?
>
> I have had some offline discussions with Franklin on this, and I am not
> fully convinced that DT is the way to go here (although I don't have the
> agreement with Franklin there).
Probably the fundamental area where we disagree is what "default" SSP
value should be used. Based on a short (< 1 ft) point to point test
using a SSP of 50% worked fine. However, I'm not convinced that this
default value of 50% will work in a more "traditional" CAN bus at higher
speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
board in even the simplest test cases.
I believe that this default SSP should be a DT property that allows any
board to determine what default value works best in general.
>
> There are two components in configuring the secondary sample point. It
> is the transceiver loopback delay and an offset (example half of the bit
> time in data phase).
>
> While the transceiver loopback delay is pretty board dependent (and thus
> amenable to DT encoding), I am not quite sure the offset can be
> configured in DT because its not really board dependent.
>
> Unfortunately, offset calculation does not seem to be an exact science.
> There are recommendations ranging from using 50% of bit time to making
> it same as the sample point configured. This means users who need to
> change the SSP due to offset variations need to change their DT even
> without anything changing on their board.
>
> Since we have a netlink socket interface to configure sample point, I
> wonder if that should be extended to configure SSP too (or at least the
> offset part of SSP)?
Sekhar is right that ideally the user should be able to set the SSP at
runtime. However, my issue is that based on my experience CAN users
expect the driver to just work the majority of times. For unique use
cases where the driver calculated values don't work then the user should
be able to override it. This should only be done for a very small
percentage of CAN users. Unless you allow DT to provide a default SSP
many users of MCAN may find that the default SSP doesn't work and must
always use runtime overrides to get anything to work. I don't think that
is a good user experience which is why I don't like the idea.
>
> Thanks,
> Sekhar
>
^ permalink raw reply
* Re: [iproute2] regression in ss output
From: Eric Dumazet @ 2017-10-18 14:17 UTC (permalink / raw)
To: Humberto Alves; +Cc: Stephen Hemminger, Phil Sutter, netdev@vger.kernel.org
In-Reply-To: <AM4PR0901MB1252D66A440ACA247AD32DF4CC4D0@AM4PR0901MB1252.eurprd09.prod.outlook.com>
On Wed, 2017-10-18 at 14:08 +0000, Humberto Alves wrote:
> Looks perfect to me. Thanks!
>
> On 10/17/2017 03:49 AM, Stephen Hemminger wrote:
> >
> > What about
> > *:80 << both IPV6 and IPV4
> > [::]:80 << IPV6_ONLY
> > 0.0.0.0:80 << IPV4_ONLY
> >
>
Yes, I missed your email Stephen, this looks a nice idea. Thanks !
^ permalink raw reply
* Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-18 14:17 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, David S. Miller, Network Development, LKML
In-Reply-To: <1508326655.2674.22.camel@sipsolutions.net>
On Wed, Oct 18, 2017 at 4:37 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2017-10-18 at 13:31 +0200, Johannes Berg wrote:
>> On Wed, 2017-10-18 at 12:29 +0200, Johannes Berg wrote:
>>
>> > Anyway, the change here looks correct to me, so I'll apply it and then
>> > perhaps clean up more. I've only changed "u16 tid" to "u8 tid" since
>> > the valid range is 0-15 (in theory, in practice 0-7).
I started with u8 tid, but I saw it cast to u16 and in a few other
places it was u16, so I went with that ultimately.
>> Well, I guess I'm clearly wrong - it's crashing our test suite left and
>> right.
>>
>> I'll dig a little bit, but I don't have much time today, and will be
>> out for a few days starting tomorrow.
>
> Ok, it's pretty obvious - you never initialize the new fields in tid_tx
> (sta and tid), only in tid_rx. Let's see if it passes with that fixed.
Argh, whoops, thanks for working on this.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH] mac80211: aggregation: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-18 14:19 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, David S. Miller, Network Development, LKML
In-Reply-To: <1508322566.2674.17.camel@sipsolutions.net>
On Wed, Oct 18, 2017 at 3:29 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>> This has been the least trivial timer conversion yet. Given the use of
>> RCU and other things I may not even know about, I'd love to get a close
>> look at this. I *think* this is correct, as it will re-lookup the tid
>> entries when firing the timer.
>
> I'm not really sure why you're doing the lookup again? That seems
> pointless, since you already have the right structure, and already rely
> on it being valid. You can't really get a new struct assigned to the
> same TID without the old one being destroyed.
I couldn't tell what the lifetime expectation was, so I left the
re-lookup. I assumed it was possible to have a timer fire after the
structure had been removed from the station structure.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH net 0/3] Fix for BPF devmap percpu allocation splat
From: Daniel Borkmann @ 2017-10-18 14:22 UTC (permalink / raw)
To: Tejun Heo
Cc: davem, ast, john.fastabend, mark.rutland, richard, sp3485, netdev,
linux-kernel, Dennis Zhou
In-Reply-To: <59E75F4F.7070706@iogearbox.net>
On 10/18/2017 04:03 PM, Daniel Borkmann wrote:
> On 10/18/2017 03:25 PM, Tejun Heo wrote:
>> Hello, Daniel.
>>
>> (cc'ing Dennis)
>>
>> On Tue, Oct 17, 2017 at 04:55:51PM +0200, Daniel Borkmann wrote:
>>> The set fixes a splat in devmap percpu allocation when we alloc
>>> the flush bitmap. Patch 1 is a prerequisite for the fix in patch 2,
>>> patch 1 is rather small, so if this could be routed via -net, for
>>> example, with Tejun's Ack that would be good. Patch 3 gets rid of
>>> remaining PCPU_MIN_UNIT_SIZE checks, which are percpu allocator
>>> internals and should not be used.
>>>
>>> Thanks!
>>>
>>> Daniel Borkmann (3):
>>> mm, percpu: add support for __GFP_NOWARN flag
>>
>> This looks fine.
>
> Great, thanks!
>
>>> bpf: fix splat for illegal devmap percpu allocation
>>> bpf: do not test for PCPU_MIN_UNIT_SIZE before percpu allocations
>>
>> These look okay too but if it helps percpu allocator can expose the
>> maximum size / alignment supported to take out the guessing game too.
>
> At least from BPF side there's right now no infra for exposing
> max possible alloc sizes for maps to e.g. user space as indication.
> There are few users left in the tree, where it would make sense for
> having some helpers though:
>
> arch/tile/kernel/setup.c:729: if (size < PCPU_MIN_UNIT_SIZE)
> arch/tile/kernel/setup.c:730: size = PCPU_MIN_UNIT_SIZE;
> drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:346: unsigned int max = (PCPU_MIN_UNIT_SIZE - sizeof(*pools)) << 3;
> drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c:352: /* make sure per cpu pool fits into PCPU_MIN_UNIT_SIZE */
> drivers/scsi/libfc/fc_exch.c:2488: /* reduce range so per cpu pool fits into PCPU_MIN_UNIT_SIZE pool */
> drivers/scsi/libfc/fc_exch.c:2489: pool_exch_range = (PCPU_MIN_UNIT_SIZE - sizeof(*pool)) /
>
>> Also, the reason why PCPU_MIN_UNIT_SIZE is what it is is because
>> nobody needed anything bigger. Increasing the size doesn't really
>> cost much at least on 64bit archs. Is that something we want to be
>> considering?
>
> For devmap (and cpumap) itself it wouldn't make sense. For per-cpu
> hashtable we could indeed consider it in the future.
Higher prio imo would be to make the allocation itself faster
though, I remember we talked about this back in May wrt hashtable,
but I kind of lost track whether there was an update on this in
the mean time. ;-)
Cheers,
Daniel
^ permalink raw reply
* [PATCH] doc: Update VRF documentation metric
From: Donald Sharp @ 2017-10-18 14:24 UTC (permalink / raw)
To: netdev, dsa
Two things:
1) Update examples to show usage of metric
2) Discuss reasoning for using such a high metric.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
---
Documentation/networking/vrf.txt | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/Documentation/networking/vrf.txt b/Documentation/networking/vrf.txt
index 3918dae..8ff7b4c 100644
--- a/Documentation/networking/vrf.txt
+++ b/Documentation/networking/vrf.txt
@@ -71,7 +71,12 @@ Setup
ip ru add iif vrf-blue table 10
3. Set the default route for the table (and hence default route for the VRF).
- ip route add table 10 unreachable default
+ ip route add table 10 unreachable default metric 4278198272
+
+ This high metric value ensures that the default unreachable route can
+ be overridden by a routing protocol suite. FRRouting interprets
+ kernel metrics as a combined admin distance (upper byte) and priority
+ (lower 3 bytes). Thus the above metric translates to [255/8192].
4. Enslave L3 interfaces to a VRF device.
ip link set dev eth1 master vrf-blue
@@ -256,7 +261,7 @@ older form without it.
For example:
$ ip route show vrf red
- prohibit default
+ unreachable default metric 4278198272
broadcast 10.2.1.0 dev eth1 proto kernel scope link src 10.2.1.2
10.2.1.0/24 dev eth1 proto kernel scope link src 10.2.1.2
local 10.2.1.2 dev eth1 proto kernel scope host src 10.2.1.2
@@ -282,7 +287,7 @@ older form without it.
ff00::/8 dev red metric 256 pref medium
ff00::/8 dev eth1 metric 256 pref medium
ff00::/8 dev eth2 metric 256 pref medium
-
+ unreachable default dev lo metric 4278198272 error -101 pref medium
8. Route Lookup for a VRF
@@ -331,7 +336,7 @@ function vrf_create
ip link add ${VRF} type vrf table ${TBID}
if [ "${VRF}" != "mgmt" ]; then
- ip route add table ${TBID} unreachable default
+ ip route add table ${TBID} unreachable default metric 4278198272
fi
ip link set dev ${VRF} up
}
--
2.9.5
^ permalink raw reply related
* Re: [PATCH] doc: Update VRF documentation metric
From: David Ahern @ 2017-10-18 14:25 UTC (permalink / raw)
To: Donald Sharp, netdev
In-Reply-To: <20171018142428.26398-1-sharpd@cumulusnetworks.com>
On 10/18/17 8:24 AM, Donald Sharp wrote:
> Two things:
>
> 1) Update examples to show usage of metric
> 2) Discuss reasoning for using such a high metric.
>
> Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
> ---
> Documentation/networking/vrf.txt | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
For net-next.
Acked-by: David Ahern <dsa@cumulusnetworks.com>
^ 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