* Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data
From: Florian Fainelli @ 2017-01-13 22:39 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, Jason Cooper, Sebastian Hesselbarth, Gregory Clement,
Russell King, Vivien Didelot, David S. Miller,
moderated list:ARM SUB-ARCHITECTURES, open list, gregkh
In-Reply-To: <20170113141110.GI10203@lunn.ch>
On 01/13/2017 06:11 AM, Andrew Lunn wrote:
>> static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev)
>> {
>> + struct dsa_chip_data *pdata = dev->platform_data;
>> struct device_node *np = dev->of_node;
>> struct dsa_switch_tree *dst;
>> struct device_node *ports;
>> u32 tree, index;
>> int i, err;
>>
>> - err = dsa_parse_member_dn(np, &tree, &index);
>> - if (err)
>> - return err;
>> + if (np) {
>> + err = dsa_parse_member_dn(np, &tree, &index);
>> + if (err)
>> + return err;
>>
>> - ports = dsa_get_ports(ds, np);
>> - if (IS_ERR(ports))
>> - return PTR_ERR(ports);
>> + ports = dsa_get_ports(ds, np);
>> + if (IS_ERR(ports))
>> + return PTR_ERR(ports);
>>
>> - err = dsa_parse_ports_dn(ports, ds);
>> - if (err)
>> - return err;
>> + err = dsa_parse_ports_dn(ports, ds);
>> + if (err)
>> + return err;
>> + } else {
>> + err = dsa_parse_member(pdata, &tree, &index);
>
Hello Andrew,
> Hi Florian
>
> Maybe it is hiding, but i don't see anywhere you check that pdata !=
> NULL.
You are right, there is not such a check, it should probably be added
early on.
>
> At least for x86 platforms, i don't expect we are booting using
> platform data like ARM systems used to do. I think it is more likely a
> glue module will be loaded. It looks up the MDIO bus and appends a
> platform data to an MDIO device. The switch driver then needs to load
> and use the platform data. But if things happen in a different order,
> it could be the switch driver probes before the glue driver, meaning
> pdata is NULL.
That's very valid, I will fix this, thanks!
>
> Do we even want to return -EPROBE_DEFERED?
I was trying to exercise that code path a little bit, but could not
quite make sense of what I was seeing, let me try again with more tracing.
--
Florian
^ permalink raw reply
* Re: [pull request][for-next] Mellanox mlx5 Reorganize core driver directory layout
From: Saeed Mahameed @ 2017-01-13 22:45 UTC (permalink / raw)
To: Tom Herbert
Cc: Leon Romanovsky, David Miller, Saeed Mahameed, Doug Ledford,
Linux Kernel Network Developers, linux-rdma
In-Reply-To: <CALx6S37hb78RJ8tCtsCsX2fg6e-D+naNpSQo6T2AyWMd0U74sQ@mail.gmail.com>
On Sat, Jan 14, 2017 at 12:06 AM, Tom Herbert <tom@herbertland.com> wrote:
> On Fri, Jan 13, 2017 at 12:29 PM, Leon Romanovsky <leon@kernel.org> wrote:
>> On Fri, Jan 13, 2017 at 12:14:07PM -0500, David Miller wrote:
>>> From: Saeed Mahameed <saeedm@mellanox.com>
>>> Date: Thu, 12 Jan 2017 19:22:34 +0200
>>>
>>> > This pull request includes one patch from Leon, this patch as described
>>> > below will change the driver directory structure and layout for better,
>>> > logical and modular driver files separation.
>>> >
>>> > This change is important to both rdma and net maintainers in order to
>>> > have smoother management of driver patches for different mlx5 sub modules
>>> > and smoother rdma-next vs. net-next features submissions.
>>> >
>>> > Please find more info below -in the tag commit message-,
>>> > review and let us know if there's any problem.
>>> >
>>> > This change doesn't introduce any conflicts with the current mlx5
>>> > fixes and cleanups posted on 2017-01-10 to net branch, and merge tests
>>> > worked flawlessly with no issues.
>>> >
>>> > This is the last pull request meant for both rdma-next and net-next.
>>> > Once pulled, this will be the base shared code for both trees.
>>>
>>> This is pretty crazy, it will make all bug fix backporting to -stable
>>> a complete nightmare for myself, Doug, various distribution maintainers
>>> and many other people who quietly have to maintain their own trees and
>>> do backporting.
>>
>> Hi Dave,
>>
>> I understand your worries, but our case is similar to various other
>> drivers, for example hfi1 which was in staging for years while
>> supported in RedHat and moved from there to IB. The Chelsio drivers did
>> similar reorg in 2016 (drivers/net/ethernet/chelsio/libcxgb) while their
>> drivers were in the tree for long time before.
>>
>> Additionally, Doug doesn't need to maintain -stable queue and it is done
>> by relevant submaintainers who are adding stable tags by themselves. In
>> the IB case, the burden will continue to be on me and not on Doug.
>>
> Recently I had to backport the mlx5 driver from 4.9 to 4.6 in order to
> get support for XDP. The biggest issue I faced was the lack of
> modularity in the many driver features that are now supported. The
> problem with backporting these new features is the spider web of
> dependencies that they bring in from the rest of the kernel. I ended
> up taking out en_rep, vxlan, en_tc, eswitch, and dcbnl. The result was
> ~340 patches which is still a lot but at least this was constrained to
> patches in the mlx5 directories and are relevant to what we want to
> do.
>
> In lieu of restructuring the directories, I would much rather see more
> config options so that we can build drivers that don't unnecessarily
> complicate our lives with features we don't use. This is not just true
> for Mellanox, but I would say it would be true of any driver that
> someone is trying to deploy and maintain at large scale.
>
I think we should have both, if the restructuring made right,
new whole features (e.g eswitch and eswitch offlaods or any independent module),
can sit in their own directory and keep their own logic concentrated
in one place, and only touch the
main driver code with simple entry points in the main flow, this way
you can simply compile their whole directories
out with a config flag directly from the Makefile.
> Btw, we did hit one issue in the backport. We started to get rx csum
> faults (checksum complete value indicates TCP checksum is bad, but
> host computation says checksum is good). I ran against 4.9 upstream
> kernel and do see these, however don't see them in 4.10. I haven't
> bisected yet. Is this a known issue?
>
Not to me, I don't recall any csum related fixes or feature submitted
lately to mlx5,
Maybe something changed in the stack ?
what configuration are you running ? what traffic ?
> Thanks,
> Tom
>
>>>
>>> I really don't think you can justify this rearrangement based upon the
>>> consequences and how much activity happens in this driver.
>>>
>>> You should have thought long and hard about the layout a long time ago
>>> rather than after the driver has been in the tree for many years.
>>>
>>> Sorry.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 net-next 0/2] mpls: Packet stats
From: Roopa Prabhu @ 2017-01-13 22:50 UTC (permalink / raw)
To: Robert Shearman; +Cc: davem, netdev, David Ahern, ebiederm
In-Reply-To: <1484331253-5908-1-git-send-email-rshearma@brocade.com>
On 1/13/17, 10:14 AM, Robert Shearman wrote:
> This patchset records per-interface packet stats in the MPLS
> forwarding path and exports them using a nest of attributes root at a
> new IFLA_STATS_AF_SPEC attribute as part of RTM_GETSTATS messages:
>
> [IFLA_STATS_AF_SPEC]
> -> [AF_MPLS]
> -> [MPLS_STATS_LINK]
> -> struct mpls_link_stats
>
> The first patch adds the rtnl infrastructure for this, including a new
> callbacks to per-AF ops of fill_stats_af and get_stats_af_size. The
> second patch records MPLS stats and makes use of the infrastructure to
> export them. The rtnl infrastructure could also be used to export IPv6
> stats in the future.
>
> Robert Shearman (2):
> net: AF-specific RTM_GETSTATS attributes
> mpls: Packet stats
>
LGTM, thanks Robert
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: AF-specific RTM_GETSTATS attributes
From: Roopa Prabhu @ 2017-01-13 22:50 UTC (permalink / raw)
To: Robert Shearman; +Cc: davem, netdev, David Ahern, ebiederm
In-Reply-To: <1484331253-5908-2-git-send-email-rshearma@brocade.com>
On 1/13/17, 10:14 AM, Robert Shearman wrote:
> Add the functionality for including address-family-specific per-link
> stats in RTM_GETSTATS messages. This is done through adding a new
> IFLA_STATS_AF_SPEC attribute under which address family attributes are
> nested and then the AF-specific attributes can be further nested. This
> follows the model of IFLA_AF_SPEC on RTM_*LINK messages and it has the
> advantage of presenting an easily extended hierarchy. The rtnl_af_ops
> structure is extended to provide AFs with the opportunity to fill and
> provide the size of their stats attributes.
>
> One alternative would have been to provide AFs with the ability to add
> attributes directly into the RTM_GETSTATS message without a nested
> hierarchy. I discounted this approach as it increases the rate at
> which the 32 attribute number space is used up and it makes
> implementation a little more tricky for stats dump resuming (at the
> moment the order in which attributes are added to the message has to
> match the numeric order of the attributes).
>
> Another alternative would have been to register per-AF RTM_GETSTATS
> handlers. I discounted this approach as I perceived a common use-case
> to be getting all the stats for an interface and this approach would
> necessitate multiple requests/dumps to retrieve them all.
>
> Signed-off-by: Robert Shearman <rshearma@brocade.com>
>
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
^ permalink raw reply
* Re: [PATCH net-next 2/2] mpls: Packet stats
From: Roopa Prabhu @ 2017-01-13 22:51 UTC (permalink / raw)
To: Robert Shearman; +Cc: davem, netdev, David Ahern, ebiederm
In-Reply-To: <1484331253-5908-3-git-send-email-rshearma@brocade.com>
On 1/13/17, 10:14 AM, Robert Shearman wrote:
> Having MPLS packet stats is useful for observing network operation and
> for diagnosing network problems. In the absence of anything better,
> RFC2863 and RFC3813 are used for guidance for which stats to expose
> and the semantics of them. In particular rx_noroutes maps to in
> unknown protos in RFC2863. The stats are exposed to userspace via
> AF_MPLS attributes embedded in the IFLA_STATS_AF_SPEC attribute of
> RTM_GETSTATS messages.
>
> All the introduced fields are 64-bit, even error ones, to ensure no
> overflow with long uptimes. Per-CPU counters are used to avoid
> cache-line contention on the commonly used fields. The other fields
> have also been made per-CPU for code to avoid performance problems in
> error conditions on the assumption that on some platforms the cost of
> atomic operations could be more expensive than sending the packet
> (which is what would be done in the success case). If that's not the
> case, we could instead not use per-CPU counters for these fields.
>
> Only unicast and non-fragment are exposed at the moment, but other
> counters can be exposed in the future either by adding to the end of
> struct mpls_link_stats or by additional netlink attributes in the
> AF_MPLS IFLA_STATS_AF_SPEC nested attribute.
>
> Signed-off-by: Robert Shearman <rshearma@brocade.com>
> ---
>
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
^ permalink raw reply
* Re: [PATCH 2/3] powerpc: bpf: flush the entire JIT buffer
From: Daniel Borkmann @ 2017-01-13 22:55 UTC (permalink / raw)
To: Naveen N. Rao, mpe; +Cc: linuxppc-dev, netdev, ast, davem
In-Reply-To: <897d01cca8cd61a42493a4f6ba6bfca056419686.1484326337.git.naveen.n.rao@linux.vnet.ibm.com>
On 01/13/2017 06:10 PM, Naveen N. Rao wrote:
> With bpf_jit_binary_alloc(), we allocate at a page granularity and fill
> the rest of the space with illegal instructions to mitigate BPF spraying
> attacks, while having the actual JIT'ed BPF program at a random location
> within the allocated space. Under this scenario, it would be better to
> flush the entire allocated buffer rather than just the part containing
> the actual program. We already flush the buffer from start to the end of
> the BPF program. Extend this to include the illegal instructions after
> the BPF program.
>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* Re: [pull request][for-next] Mellanox mlx5 Reorganize core driver directory layout
From: Tom Herbert @ 2017-01-13 22:56 UTC (permalink / raw)
To: Saeed Mahameed
Cc: Leon Romanovsky, David Miller, Saeed Mahameed, Doug Ledford,
Linux Kernel Network Developers,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CALzJLG_RsvgszVxGZ7+VgYjET9w=XTjq36Z72diOQFNJWz9tQw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Jan 13, 2017 at 2:45 PM, Saeed Mahameed
<saeedm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> On Sat, Jan 14, 2017 at 12:06 AM, Tom Herbert <tom-BjP2VixgY4xUbtYUoyoikg@public.gmane.org> wrote:
>> On Fri, Jan 13, 2017 at 12:29 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>> On Fri, Jan 13, 2017 at 12:14:07PM -0500, David Miller wrote:
>>>> From: Saeed Mahameed <saeedm-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>> Date: Thu, 12 Jan 2017 19:22:34 +0200
>>>>
>>>> > This pull request includes one patch from Leon, this patch as described
>>>> > below will change the driver directory structure and layout for better,
>>>> > logical and modular driver files separation.
>>>> >
>>>> > This change is important to both rdma and net maintainers in order to
>>>> > have smoother management of driver patches for different mlx5 sub modules
>>>> > and smoother rdma-next vs. net-next features submissions.
>>>> >
>>>> > Please find more info below -in the tag commit message-,
>>>> > review and let us know if there's any problem.
>>>> >
>>>> > This change doesn't introduce any conflicts with the current mlx5
>>>> > fixes and cleanups posted on 2017-01-10 to net branch, and merge tests
>>>> > worked flawlessly with no issues.
>>>> >
>>>> > This is the last pull request meant for both rdma-next and net-next.
>>>> > Once pulled, this will be the base shared code for both trees.
>>>>
>>>> This is pretty crazy, it will make all bug fix backporting to -stable
>>>> a complete nightmare for myself, Doug, various distribution maintainers
>>>> and many other people who quietly have to maintain their own trees and
>>>> do backporting.
>>>
>>> Hi Dave,
>>>
>>> I understand your worries, but our case is similar to various other
>>> drivers, for example hfi1 which was in staging for years while
>>> supported in RedHat and moved from there to IB. The Chelsio drivers did
>>> similar reorg in 2016 (drivers/net/ethernet/chelsio/libcxgb) while their
>>> drivers were in the tree for long time before.
>>>
>>> Additionally, Doug doesn't need to maintain -stable queue and it is done
>>> by relevant submaintainers who are adding stable tags by themselves. In
>>> the IB case, the burden will continue to be on me and not on Doug.
>>>
>> Recently I had to backport the mlx5 driver from 4.9 to 4.6 in order to
>> get support for XDP. The biggest issue I faced was the lack of
>> modularity in the many driver features that are now supported. The
>> problem with backporting these new features is the spider web of
>> dependencies that they bring in from the rest of the kernel. I ended
>> up taking out en_rep, vxlan, en_tc, eswitch, and dcbnl. The result was
>> ~340 patches which is still a lot but at least this was constrained to
>> patches in the mlx5 directories and are relevant to what we want to
>> do.
>>
>> In lieu of restructuring the directories, I would much rather see more
>> config options so that we can build drivers that don't unnecessarily
>> complicate our lives with features we don't use. This is not just true
>> for Mellanox, but I would say it would be true of any driver that
>> someone is trying to deploy and maintain at large scale.
>>
>
> I think we should have both, if the restructuring made right,
> new whole features (e.g eswitch and eswitch offlaods or any independent module),
> can sit in their own directory and keep their own logic concentrated
> in one place, and only touch the
> main driver code with simple entry points in the main flow, this way
> you can simply compile their whole directories
> out with a config flag directly from the Makefile.
>
>> Btw, we did hit one issue in the backport. We started to get rx csum
>> faults (checksum complete value indicates TCP checksum is bad, but
>> host computation says checksum is good). I ran against 4.9 upstream
>> kernel and do see these, however don't see them in 4.10. I haven't
>> bisected yet. Is this a known issue?
>>
>
> Not to me, I don't recall any csum related fixes or feature submitted
> lately to mlx5,
> Maybe something changed in the stack ?
>
> what configuration are you running ? what traffic ?
>
Nothing fancy. 8 queues and 20 concurrent netperf TCP_STREAMs trips
it. Not a lot of them, but I don't think we really should ever see
these errors.
Tom
>> Thanks,
>> Tom
>>
>>>>
>>>> I really don't think you can justify this rearrangement based upon the
>>>> consequences and how much activity happens in this driver.
>>>>
>>>> You should have thought long and hard about the layout a long time ago
>>>> rather than after the driver has been in the tree for many years.
>>>>
>>>> Sorry.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [pull request][for-next] Mellanox mlx5 Reorganize core driver directory layout
From: Rick Jones @ 2017-01-13 22:59 UTC (permalink / raw)
To: Tom Herbert, Saeed Mahameed
Cc: Leon Romanovsky, David Miller, Saeed Mahameed, Doug Ledford,
Linux Kernel Network Developers,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CALx6S35cSnMoBtxi+Eb_mH=LCLy9yY6U5sxofndce9r0eCs8+w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 01/13/2017 02:56 PM, Tom Herbert wrote:
> On Fri, Jan 13, 2017 at 2:45 PM, Saeed Mahameed
>> what configuration are you running ? what traffic ?
>>
> Nothing fancy. 8 queues and 20 concurrent netperf TCP_STREAMs trips
> it. Not a lot of them, but I don't think we really should ever see
> these errors.
Straight-up defaults with netperf, or do you use specific -s/S or -m/M
options?
happy benchmarking,
rick jones
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net] mlx4: do not call napi_schedule() without care
From: Alexander Duyck @ 2017-01-13 23:07 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Erez Shitrit, Eugenia Emantayev,
Tariq Toukan
In-Reply-To: <1484325564.13165.30.camel@edumazet-glaptop3.roam.corp.google.com>
On Fri, Jan 13, 2017 at 8:39 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Disable BH around the call to napi_schedule() to avoid following warning
>
> [ 52.095499] NOHZ: local_softirq_pending 08
> [ 52.421291] NOHZ: local_softirq_pending 08
> [ 52.608313] NOHZ: local_softirq_pending 08
>
> Fixes: 8d59de8f7bb3 ("net/mlx4_en: Process all completions in RX rings after port goes up")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Erez Shitrit <erezsh@mellanox.com>
> Cc: Eugenia Emantayev <eugenia@mellanox.com>
> Cc: Tariq Toukan <tariqt@mellanox.com>
> ---
> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 4910d9af19335d4b97d39760c163b41eecc26242..761f8b12399cab245abccc0f7d7f84fde742c14d 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -1748,8 +1748,11 @@ int mlx4_en_start_port(struct net_device *dev)
> /* Process all completions if exist to prevent
> * the queues freezing if they are full
> */
> - for (i = 0; i < priv->rx_ring_num; i++)
> + for (i = 0; i < priv->rx_ring_num; i++) {
> + local_bh_disable();
> napi_schedule(&priv->rx_cq[i]->napi);
> + local_bh_enable();
> + }
Couldn't you save yourself a ton of trouble by wrapping the loop
inside of the local_bh_disable/enable instead of wrapping them up
inside the loop? It just seems like it might be more efficient to
schedule them and then process them as a block instead of doing it one
at a time.
- Alex
^ permalink raw reply
* Re: [pull request][for-next] Mellanox mlx5 Reorganize core driver directory layout
From: Tom Herbert @ 2017-01-13 23:07 UTC (permalink / raw)
To: Rick Jones
Cc: Saeed Mahameed, Leon Romanovsky, David Miller, Saeed Mahameed,
Doug Ledford, Linux Kernel Network Developers,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <10e998b7-0008-2f26-2874-57ec61c8cacb-ZPxbGqLxI0U@public.gmane.org>
On Fri, Jan 13, 2017 at 2:59 PM, Rick Jones <rick.jones2-ZPxbGqLxI0U@public.gmane.org> wrote:
> On 01/13/2017 02:56 PM, Tom Herbert wrote:
>>
>> On Fri, Jan 13, 2017 at 2:45 PM, Saeed Mahameed
>>>
>>> what configuration are you running ? what traffic ?
>>>
>> Nothing fancy. 8 queues and 20 concurrent netperf TCP_STREAMs trips
>> it. Not a lot of them, but I don't think we really should ever see
>> these errors.
>
>
> Straight-up defaults with netperf, or do you use specific -s/S or -m/M
> options?
>
./super_netperf_tput 20 -H test001 -l 100 -t TCP_STREAM
> happy benchmarking,
>
> rick jones
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 7/8] net: Rename TCA*BPF_DIGEST to ..._SHA256
From: Daniel Borkmann @ 2017-01-13 23:08 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Lutomirski, Netdev, LKML, Linux Crypto Mailing List,
Jason A. Donenfeld, Hannes Frederic Sowa, Alexei Starovoitov,
Eric Dumazet, Eric Biggers, Tom Herbert, David S. Miller,
Alexei Starovoitov
In-Reply-To: <CALCETrVhuszdsfayLrBBkSzJ+A3m+hJjdZQWDj9FOg+2UB_ZBw@mail.gmail.com>
On 01/11/2017 07:19 PM, Andy Lutomirski wrote:
> On Wed, Jan 11, 2017 at 1:09 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
[...]
>> Ok. Sleeping over this a bit, how about a general rename into
>> "prog_tag" for fdinfo and TCA_BPF_TAG resp. TCA_ACT_BPF_TAG for
>> the netlink attributes, fwiw, it might reduce any assumptions on
>> this being made? If this would be preferable, I could cook that
>> patch against -net for renaming it?
>
> That would be fine with me.
>
> I think there are two reasonable approaches to computing the actual tag.
>
> 1. Use a standard, modern cryptographic hash. SHA-256, SHA-512,
> Blake2b, whatever. SHA-1 is a bad choice in part because it's partly
> broken and in part because the implementation in lib/ is a real mess
> to use (as you noticed while writing the code).
>
> 2. Use whatever algorithm you like but make the tag so short that it's
> obviously not collision-free. 48 or 64 bits is probably reasonable.
>
> The intermediate versions are just asking for trouble.
Yeah agree, I've just sent a patch to rework this a bit and it got
also reasonably small for net. Cleanups, if needed, can be done in
net-next once that's pulled into it.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net] bpf: rework prog_digest into prog_tag
From: Andy Lutomirski @ 2017-01-13 23:16 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Alexei Starovoitov, Andrew Lutomirski,
Network Development
In-Reply-To: <384476e29f6a378766c4052187b9b5f840f4030e.1484346362.git.daniel@iogearbox.net>
On Fri, Jan 13, 2017 at 2:38 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> Commit 7bd509e311f4 ("bpf: add prog_digest and expose it via
> fdinfo/netlink") was recently discussed, partially due to
> admittedly suboptimal name of "prog_digest" in combination
> with sha1 hash usage, thus inevitably and rightfully concerns
> about its security in terms of collision resistance were
> raised with regards to use-cases.
>
Seems reasonable. My only question is whether you'd still want to
switch to SHA-256 just from a code cleanliness perspective. With
SHA-256 you can use the easy streaming API I wrote, but with SHA-1
you're still stuck with the crappy API in lib/, and I'm not
volunteering to fix up the SHA-1 API.
--Andy
^ permalink raw reply
* Re: [PATCH net] bpf: rework prog_digest into prog_tag
From: Alexei Starovoitov @ 2017-01-13 23:34 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Borkmann, David S. Miller, Andrew Lutomirski,
Network Development
In-Reply-To: <CALCETrUjmscreaadCOn6hSEMBEf83JyhwdVtKj1=fAf3MuiDtQ@mail.gmail.com>
On Fri, Jan 13, 2017 at 03:16:44PM -0800, Andy Lutomirski wrote:
> On Fri, Jan 13, 2017 at 2:38 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> > Commit 7bd509e311f4 ("bpf: add prog_digest and expose it via
> > fdinfo/netlink") was recently discussed, partially due to
> > admittedly suboptimal name of "prog_digest" in combination
> > with sha1 hash usage, thus inevitably and rightfully concerns
> > about its security in terms of collision resistance were
> > raised with regards to use-cases.
> >
>
> Seems reasonable. My only question is whether you'd still want to
> switch to SHA-256 just from a code cleanliness perspective. With
> SHA-256 you can use the easy streaming API I wrote, but with SHA-1
> you're still stuck with the crappy API in lib/, and I'm not
> volunteering to fix up the SHA-1 API.
No. As was stated many times before there are only negatives
in switching to sha256.
^ permalink raw reply
* Re: [PATCH net] mlx4: do not call napi_schedule() without care
From: Eric Dumazet @ 2017-01-13 23:40 UTC (permalink / raw)
To: Alexander Duyck
Cc: David Miller, netdev, Erez Shitrit, Eugenia Emantayev,
Tariq Toukan
In-Reply-To: <CAKgT0UdgzU1BAjiKyMFeB+KOJbDSdbBS6KdHX2LZ_pWT3XiXjg@mail.gmail.com>
On Fri, 2017-01-13 at 15:07 -0800, Alexander Duyck wrote:
> On Fri, Jan 13, 2017 at 8:39 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Disable BH around the call to napi_schedule() to avoid following warning
> >
> > [ 52.095499] NOHZ: local_softirq_pending 08
> > [ 52.421291] NOHZ: local_softirq_pending 08
> > [ 52.608313] NOHZ: local_softirq_pending 08
> >
> > Fixes: 8d59de8f7bb3 ("net/mlx4_en: Process all completions in RX rings after port goes up")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Erez Shitrit <erezsh@mellanox.com>
> > Cc: Eugenia Emantayev <eugenia@mellanox.com>
> > Cc: Tariq Toukan <tariqt@mellanox.com>
> > ---
> > drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> > index 4910d9af19335d4b97d39760c163b41eecc26242..761f8b12399cab245abccc0f7d7f84fde742c14d 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> > @@ -1748,8 +1748,11 @@ int mlx4_en_start_port(struct net_device *dev)
> > /* Process all completions if exist to prevent
> > * the queues freezing if they are full
> > */
> > - for (i = 0; i < priv->rx_ring_num; i++)
> > + for (i = 0; i < priv->rx_ring_num; i++) {
> > + local_bh_disable();
> > napi_schedule(&priv->rx_cq[i]->napi);
> > + local_bh_enable();
> > + }
>
> Couldn't you save yourself a ton of trouble by wrapping the loop
> inside of the local_bh_disable/enable instead of wrapping them up
> inside the loop? It just seems like it might be more efficient to
> schedule them and then process them as a block instead of doing it one
> at a time.
What kind of troubles ?
Given the problem might be happening under flood, I believe it is much
safer to do as I did.
Otherwise, we will have to process a ton of messages at the
local_bh_enable() time and lock the {softirq}IRQ on one cpu.
I chose to do this on purpose.
Batching can be dangerous, and this is exactly the point we do not want
batching, with say 64 queues.
This code is driver starts, hardly fast path.
^ permalink raw reply
* Re: [PATCH net] bpf: rework prog_digest into prog_tag
From: Daniel Borkmann @ 2017-01-13 23:41 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David S. Miller, Alexei Starovoitov, Andrew Lutomirski,
Network Development
In-Reply-To: <CALCETrUjmscreaadCOn6hSEMBEf83JyhwdVtKj1=fAf3MuiDtQ@mail.gmail.com>
On 01/14/2017 12:16 AM, Andy Lutomirski wrote:
> On Fri, Jan 13, 2017 at 2:38 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> Commit 7bd509e311f4 ("bpf: add prog_digest and expose it via
>> fdinfo/netlink") was recently discussed, partially due to
>> admittedly suboptimal name of "prog_digest" in combination
>> with sha1 hash usage, thus inevitably and rightfully concerns
>> about its security in terms of collision resistance were
>> raised with regards to use-cases.
>
> Seems reasonable. My only question is whether you'd still want to
> switch to SHA-256 just from a code cleanliness perspective. With
> SHA-256 you can use the easy streaming API I wrote, but with SHA-1
> you're still stuck with the crappy API in lib/, and I'm not
> volunteering to fix up the SHA-1 API.
We'd need to truncate that in kernel anyway to not get a too long
tag, so given that I'm actually fine with it as-is. I was planning
to submit the code for testing to bpf selftests for net-next once
it's merged back, too.
Thanks,
Daniel
^ permalink raw reply
* [PATCH next] ipvlan: fix dev_id creation corner case.
From: Mahesh Bandewar @ 2017-01-13 23:48 UTC (permalink / raw)
To: David Miller
Cc: netdev, Mahesh Bandewar, Mahesh Bandewar, David Miller,
Eric Dumazet
From: Mahesh Bandewar <maheshb@google.com>
In the last patch da36e13cf65 ("ipvlan: improvise dev_id generation
logic in IPvlan") I missed some part of Dave's suggestion and because
of that the dev_id creation could fail in a corner case scenario. This
would happen when more or less 64k devices have been already created and
several have been deleted. If the devices that are still sticking around
are the last n bits from the bitmap. So in this scenario even if lower
bits are available, the dev_id search is so narrow that it always fails.
Fixes: da36e13cf65 ("ipvlan: improvise dev_id generation logic in IPvlan")
CC: David Miller <davem@davemloft.org>
CC: Eric Dumazet <edumazet@google.com>
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
drivers/net/ipvlan/ipvlan_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 92b221a03350..b5c390f0f2b3 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -550,6 +550,9 @@ static int ipvlan_link_new(struct net *src_net, struct net_device *dev,
err = ida_simple_get(&port->ida, port->dev_id_start, 0xFFFE,
GFP_KERNEL);
if (err < 0)
+ err = ida_simple_get(&port->ida, 0x1, port->dev_id_start,
+ GFP_KERNEL);
+ if (err < 0)
goto destroy_ipvlan_port;
dev->dev_id = err;
/* Increment id-base to the next slot for the future assignment */
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: [PATCH net] bpf: rework prog_digest into prog_tag
From: Andy Lutomirski @ 2017-01-13 23:49 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David S. Miller, Alexei Starovoitov, Andrew Lutomirski,
Network Development
In-Reply-To: <58796595.3030904@iogearbox.net>
On Fri, Jan 13, 2017 at 3:41 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 01/14/2017 12:16 AM, Andy Lutomirski wrote:
>>
>> On Fri, Jan 13, 2017 at 2:38 PM, Daniel Borkmann <daniel@iogearbox.net>
>> wrote:
>>>
>>> Commit 7bd509e311f4 ("bpf: add prog_digest and expose it via
>>> fdinfo/netlink") was recently discussed, partially due to
>>> admittedly suboptimal name of "prog_digest" in combination
>>> with sha1 hash usage, thus inevitably and rightfully concerns
>>> about its security in terms of collision resistance were
>>> raised with regards to use-cases.
>>
>>
>> Seems reasonable. My only question is whether you'd still want to
>> switch to SHA-256 just from a code cleanliness perspective. With
>> SHA-256 you can use the easy streaming API I wrote, but with SHA-1
>> you're still stuck with the crappy API in lib/, and I'm not
>> volunteering to fix up the SHA-1 API.
>
>
> We'd need to truncate that in kernel anyway to not get a too long
> tag, so given that I'm actually fine with it as-is. I was planning
> to submit the code for testing to bpf selftests for net-next once
> it's merged back, too.
Unless you want to kill off that vmalloc()+vfree() pair...
--Andy
^ permalink raw reply
* [PATCH net-next] net: ipv6: Allow shorthand delete of all nexthops in multipath route
From: David Ahern @ 2017-01-13 23:54 UTC (permalink / raw)
To: netdev; +Cc: ddutt, David Ahern
IPv4 allows multipath routes to be deleted using just the prefix and
length. For example:
$ ip ro ls vrf red
unreachable default metric 8192
1.1.1.0/24
nexthop via 10.100.1.254 dev eth1 weight 1
nexthop via 10.11.200.2 dev eth11.200 weight 1
10.11.200.0/24 dev eth11.200 proto kernel scope link src 10.11.200.3
10.100.1.0/24 dev eth1 proto kernel scope link src 10.100.1.3
$ ip ro del 1.1.1.0/24 vrf red
$ ip ro ls vrf red
unreachable default metric 8192
10.11.200.0/24 dev eth11.200 proto kernel scope link src 10.11.200.3
10.100.1.0/24 dev eth1 proto kernel scope link src 10.100.1.3
The same notation does not work with IPv6 because of how multipath routes
are implemented for IPv6. For IPv6 only the first nexthop of a multipath
route is deleted if the request contains only a prefix and length. This
leads to unnecessary complexity in userspace dealing with IPv6 multipath
routes.
This patch allows all nexthops to be deleted without specifying each one
in the delete request by passing a new flag, RTM_F_ALL_NEXTHOPS, in
rtm_flags.
With this patch (and an updated iproute2 command):
$ ip -6 ro ls vrf red
1111::/120 via 2100:1::62 dev eth1 metric 1024 pref medium
1111::/120 via 2100:1::61 dev eth1 metric 1024 pref medium
1111::/120 via 2100:1::60 dev eth1 metric 1024 pref medium
2100:1::/120 dev eth1 proto kernel metric 256 pref medium
2100:1::/64 dev eth1 proto kernel metric 256 expires 86386sec pref medium
...
$ ip -6 ro del vrf red 1111::1/120
$ ip -6 ro ls vrf red
2100:1::/120 dev eth1 proto kernel metric 256 pref medium
2100:1::/64 dev eth1 proto kernel metric 256 expires 86382sec pref medium
...
The flag is added to fib6_config by converting fc_type to a u16 (as
noted fc_type only uses 8 bits). The new u16 hole is a bitmap with
fc_delete_all_nexthop as the first bit.
Suggested-by: Dinesh Dutt <ddutt@cumulusnetworks.com>
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
include/net/ip6_fib.h | 4 +++-
include/uapi/linux/rtnetlink.h | 1 +
net/ipv6/route.c | 10 +++++++++-
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index a74e2aa40ef4..11ab99e87c5f 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -37,7 +37,9 @@ struct fib6_config {
int fc_ifindex;
u32 fc_flags;
u32 fc_protocol;
- u32 fc_type; /* only 8 bits are used */
+ u16 fc_type; /* only 8 bits are used */
+ u16 fc_delete_all_nexthop : 1,
+ __unused : 15;
struct in6_addr fc_dst;
struct in6_addr fc_src;
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 8c93ad1ef9ab..7fb206bc42f9 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -276,6 +276,7 @@ enum rt_scope_t {
#define RTM_F_EQUALIZE 0x400 /* Multipath equalizer: NI */
#define RTM_F_PREFIX 0x800 /* Prefix addresses */
#define RTM_F_LOOKUP_TABLE 0x1000 /* set rtm_table to FIB lookup result */
+#define RTM_F_ALL_NEXTHOPS 0x2000 /* delete all nexthops (IPv6) */
/* Reserved table identifiers */
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ce5aaf448c54..8bb5f6a35ba8 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2154,6 +2154,7 @@ static int ip6_route_del(struct fib6_config *cfg)
if (!table)
return err;
+again:
read_lock_bh(&table->tb6_lock);
fn = fib6_locate(&table->tb6_root,
@@ -2179,7 +2180,11 @@ static int ip6_route_del(struct fib6_config *cfg)
dst_hold(&rt->dst);
read_unlock_bh(&table->tb6_lock);
- return __ip6_del_rt(rt, &cfg->fc_nlinfo);
+ err = __ip6_del_rt(rt, &cfg->fc_nlinfo);
+ if (err || !cfg->fc_delete_all_nexthop)
+ return err;
+
+ goto again;
}
}
read_unlock_bh(&table->tb6_lock);
@@ -2849,6 +2854,9 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
if (rtm->rtm_flags & RTM_F_CLONED)
cfg->fc_flags |= RTF_CACHE;
+ if (rtm->rtm_flags & RTM_F_ALL_NEXTHOPS)
+ cfg->fc_delete_all_nexthop = 1;
+
cfg->fc_nlinfo.portid = NETLINK_CB(skb).portid;
cfg->fc_nlinfo.nlh = nlh;
cfg->fc_nlinfo.nl_net = sock_net(skb->sk);
--
2.1.4
^ permalink raw reply related
* Re: [Patch net] atm: remove an unnecessary loop
From: Chas Williams @ 2017-01-13 23:54 UTC (permalink / raw)
To: Cong Wang, David Miller
Cc: Francois Romieu, Linux Kernel Network Developers, Michal Hocko,
Andrey Konovalov
In-Reply-To: <CAM_iQpXd8qTTRT_+CkHnpMNC=CSS9vfB6zKtSjmLV_yya5BL8Q@mail.gmail.com>
On Fri, 2017-01-13 at 10:20 -0800, Cong Wang wrote:
> On Fri, Jan 13, 2017 at 9:10 AM, David Miller <davem@davemloft.net> wrote:
> > From: Francois Romieu <romieu@fr.zoreil.com>
> > Date: Fri, 13 Jan 2017 01:07:00 +0100
> >
> >> Were alloc_skb moved one level up in the call stack, there would be
> >> no need to use the new wait api in the subsequent page, thus easing
> >> pre 3.19 longterm kernel maintenance (at least those on korg page).
> >>
> >> But it tastes a tad bit too masochistic.
> >
> > Lack of error handling of allocation failure is always a huge red
> > flag. We even long ago tried to do something like this for TCP FIN
> > handling.
> >
> > It's dumb, it doesn't work.
> >
> > Therefore I agree that the correct fix is to move the SKB allocation
> > up one level to vcc_sendmsg() and make it handle errors properly.
>
> If you can justify API is not broken by doing that, I am more than happy
> to do it, as I already stated in the latter patch:
The man page for sendmsg() allows for ENOMEM. See below.
>
> "Of course, the logic itself is suspicious, other sendmsg()
> could handle skb allocation failure very well, not sure
> why ATM has to wait for a successful one here. But probably
> it is too late to change since the errno and behavior is
> visible to user-space. So just leave the logic as it is."
>
> For some reason, no one reads that patch. :-/
I read it and I agree. I think it should be moved up/conflated with
vcc_sendmsg(). vcc_sendmsg() can already return an errno for other
conditions so if so has written something where they are explicitly
not expecting a ENOMEM, we really can't help them.
I would certainly prefer to not have to resort to an atomic allocation.
That's just going to make matters worse as far as similarity to the
existing API.
So, as Francois has suggested, just wait for the atm socket to
drain, and then do the allocation after the wait is finished.
^ permalink raw reply
* Re: [net PATCH v3 2/5] net: virtio: wrap rtnl_lock in test for calling with lock already held
From: John Fastabend @ 2017-01-13 23:56 UTC (permalink / raw)
To: Stephen Hemminger
Cc: jasowang, mst, john.r.fastabend, netdev, alexei.starovoitov,
daniel
In-Reply-To: <58790F02.8090409@gmail.com>
On 17-01-13 09:31 AM, John Fastabend wrote:
> On 17-01-13 08:34 AM, Stephen Hemminger wrote:
>> On Thu, 12 Jan 2017 18:51:00 -0800
>> John Fastabend <john.fastabend@gmail.com> wrote:
>>
>>>
>>> -static void free_receive_bufs(struct virtnet_info *vi)
>>> +static void free_receive_bufs(struct virtnet_info *vi, bool need_lock)
>>> {
>>> struct bpf_prog *old_prog;
>>> int i;
>>>
>>> - rtnl_lock();
>>> + if (need_lock)
>>> + rtnl_lock();
>>> for (i = 0; i < vi->max_queue_pairs; i++) {
>>> while (vi->rq[i].pages)
>>> __free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0);
>>> @@ -1879,7 +1880,8 @@ static void free_receive_bufs(struct virtnet_info *vi)
>>> if (old_prog)
>>> bpf_prog_put(old_prog);
>>> }
>>> - rtnl_unlock();
>>> + if (need_lock)
>>> + rtnl_unlock();
>>> }
>>
>> Conditional locking is bad idea; sparse complains about it and is later source
>> of bugs. The more typical way of doing this in kernel is:
>
> OK I'll use the normal form.
>
>>
>> void _foo(some args)
>> {
>> ASSERT_RTNL();
>>
>> ...
>> }
>>
>> void foo(some args)
>> {
>> rtnl_lock();
>> _foo(some args)
>> rtnl_unlock();
>> }
>>
>>
>
Actually doing this without a rtnl_try_lock() is going to create two more
callbacks in virtio core just for virtio_net. All the other users do not
appear to have locking restrictions. How about the following it at least
helps in that there is no argument passing and if/else on the locks itself
but does use the if around rtnl_try_lock().
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1864,12 +1864,11 @@ static void virtnet_free_queues(struct virtnet_info *vi)
kfree(vi->sq);
}
-static void free_receive_bufs(struct virtnet_info *vi)
+static void _free_receive_bufs(struct virtnet_info *vi)
{
struct bpf_prog *old_prog;
int i;
- rtnl_lock();
for (i = 0; i < vi->max_queue_pairs; i++) {
while (vi->rq[i].pages)
__free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0);
@@ -1879,6 +1878,12 @@ static void free_receive_bufs(struct virtnet_info *vi)
if (old_prog)
bpf_prog_put(old_prog);
}
+}
+
+static void free_receive_bufs(struct virtnet_info *vi)
+{
+ rtnl_lock();
+ _free_receive_bufs(vi);
rtnl_unlock();
}
@@ -2358,7 +2363,10 @@ static void remove_vq_common(struct virtnet_info *vi)
/* Free unused buffers in both send and recv, if any. */
free_unused_bufs(vi);
- free_receive_bufs(vi);
+ if (rtnl_is_locked());
+ _free_receive_bufs(vi);
+ else
+ free_receive_bufs(vi);
free_receive_page_frags(vi);
^ permalink raw reply
* Re: [PATCH net] bpf: rework prog_digest into prog_tag
From: Daniel Borkmann @ 2017-01-13 23:59 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David S. Miller, Alexei Starovoitov, Andrew Lutomirski,
Network Development
In-Reply-To: <CALCETrWD7FuRLH0dC46bMf85mXosC3r54=TAXdar5EsZvbvyzg@mail.gmail.com>
On 01/14/2017 12:49 AM, Andy Lutomirski wrote:
> On Fri, Jan 13, 2017 at 3:41 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 01/14/2017 12:16 AM, Andy Lutomirski wrote:
>>> On Fri, Jan 13, 2017 at 2:38 PM, Daniel Borkmann <daniel@iogearbox.net>
>>> wrote:
>>>>
>>>> Commit 7bd509e311f4 ("bpf: add prog_digest and expose it via
>>>> fdinfo/netlink") was recently discussed, partially due to
>>>> admittedly suboptimal name of "prog_digest" in combination
>>>> with sha1 hash usage, thus inevitably and rightfully concerns
>>>> about its security in terms of collision resistance were
>>>> raised with regards to use-cases.
>>>
>>> Seems reasonable. My only question is whether you'd still want to
>>> switch to SHA-256 just from a code cleanliness perspective. With
>>> SHA-256 you can use the easy streaming API I wrote, but with SHA-1
>>> you're still stuck with the crappy API in lib/, and I'm not
>>> volunteering to fix up the SHA-1 API.
>>
>> We'd need to truncate that in kernel anyway to not get a too long
>> tag, so given that I'm actually fine with it as-is. I was planning
>> to submit the code for testing to bpf selftests for net-next once
>> it's merged back, too.
>
> Unless you want to kill off that vmalloc()+vfree() pair...
That is really just in slow-path, and should that become a bottleneck
compared to the rest of the verification steps or allocs we do there,
then we can always clean it up in net-next.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v2 net-next] Introduce a sysctl that modifies the value of PROT_SOCK.
From: Krister Johansen @ 2017-01-14 0:11 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Krister Johansen, Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1484231997.15816.36.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, Jan 12, 2017 at 06:39:57AM -0800, Eric Dumazet wrote:
> On Wed, 2017-01-11 at 22:52 -0800, Krister Johansen wrote:
> > Add net.ipv4.ip_unprotected_port_start, which is a per namespace sysctl
> > that denotes the first unprotected inet port in the namespace. To
> > disable all protected ports set this to zero. It also checks for
> > overlap with the local port range. The protected and local range may
> > not overlap.
> >
> > The use case for this change is to allow containerized processes to bind
> > to priviliged ports, but prevent them from ever being allowed to modify
> > their container's network configuration. The latter is accomplished by
> > ensuring that the network namespace is not a child of the user
> > namespace. This modification was needed to allow the container manager
> > to disable a namespace's priviliged port restrictions without exposing
> > control of the network namespace to processes in the user namespace.
> >
> > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> > ---
> > include/net/ip.h | 10 +++++++++
> > include/net/netns/ipv4.h | 1 +
> > net/ipv4/af_inet.c | 5 ++++-
> > net/ipv4/sysctl_net_ipv4.c | 50 +++++++++++++++++++++++++++++++++++++++++-
> > net/ipv6/af_inet6.c | 3 ++-
> > net/netfilter/ipvs/ip_vs_ctl.c | 7 +++---
> > net/sctp/socket.c | 10 +++++----
> > security/selinux/hooks.c | 3 ++-
>
> Adding a new sysctl without documentation is generally not accepted.
>
> Please take a look at Documentation/networking/ip-sysctl.txt
Thanks for catching this. I'll add an entry to the documentation.
> BTW, sticking to 'unprivileged' ports might be better than 'unprotected'
> which is vague.
I don't have a strong preference about the naming. I'd be happy to
change it to 'unprivileged' instead.
-K
^ permalink raw reply
* Re: [PATCH v2 net-next] Introduce a sysctl that modifies the value of PROT_SOCK.
From: Krister Johansen @ 2017-01-14 0:13 UTC (permalink / raw)
To: David Miller; +Cc: kjlx, stephen, netdev
In-Reply-To: <20170112.092213.864894939381841760.davem@davemloft.net>
On Thu, Jan 12, 2017 at 09:22:13AM -0500, David Miller wrote:
> From: Krister Johansen <kjlx@templeofstupid.com>
> > The use case for this change is to allow containerized processes to bind
> > to priviliged ports, but prevent them from ever being allowed to modify
> > their container's network configuration. The latter is accomplished by
> > ensuring that the network namespace is not a child of the user
> > namespace. This modification was needed to allow the container manager
> > to disable a namespace's priviliged port restrictions without exposing
> > control of the network namespace to processes in the user namespace.
>
> This is what CAP_NET_BIND_SERVICE is for, and why it is a separate
> network privilege, please use it.
It sounds like I may have done an inadequate job of explaining why I
took this approach instead of going the CAP_NET_BIND_SERVICE route.
In this scenario, the network namespace is created and configured first.
Then the containerized processed get placed into a separate user
namespace. This is so that the processes in the container, even if they
somehow manage to obtain extra privilege in the userns, can never modify
the network namespace.
The check in ns_capable() is looking at the priviliges of the user
namespace that created the netns and its parents. Even if I were to
grant a process in the container CAP_NET_BIND_SERVICE, ns_capable()
wouldn't recognize that as being a valid privilige for the netns.
If I were to invert the order of operations and create the userns before
the netns, then the capability would be recognized. However, that also
allows any potential privilege escalation in the userns to bring with it
the potential that an attacker can modify the container's network
configuration.
I'd much rather run the containers without privs, and without the userns
having rights to the netns, to mitigate the risk of an attacker being
able to alter the container's networking configuration.
-K
^ permalink raw reply
* Re: [Patch net] atm: remove an unnecessary loop
From: Francois Romieu @ 2017-01-14 0:14 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Linux Kernel Network Developers, Michal Hocko,
Chas Williams, Andrey Konovalov
In-Reply-To: <CAM_iQpXd8qTTRT_+CkHnpMNC=CSS9vfB6zKtSjmLV_yya5BL8Q@mail.gmail.com>
Cong Wang <xiyou.wangcong@gmail.com> :
[...]
> If you can justify API is not broken by doing that, I am more than happy
> to do it, as I already stated in the latter patch:
>
> "Of course, the logic itself is suspicious, other sendmsg()
> could handle skb allocation failure very well, not sure
> why ATM has to wait for a successful one here. But probably
> it is too late to change since the errno and behavior is
> visible to user-space. So just leave the logic as it is."
>
> For some reason, no one reads that patch. :-/
Believe it or not but I actually read it.
It changes the logic : the original code would have been unable to
escape the while loop on memory failure. Fine, I don't mind the change.
Actually I believe that these two patches are too shy (and backport
unefficient). Instead of trying to reformulate why, here's what I have
in mind. Uncompiled, caveat emptor, etc.
I'll do a (slow) build and test on saturday's night with a pair of
iphase 5575.
diff --git a/net/atm/common.c b/net/atm/common.c
index a3ca922..67f76f3 100644
--- a/net/atm/common.c
+++ b/net/atm/common.c
@@ -62,21 +62,16 @@ static void vcc_remove_socket(struct sock *sk)
write_unlock_irq(&vcc_sklist_lock);
}
-static struct sk_buff *alloc_tx(struct atm_vcc *vcc, unsigned int size)
+static bool vcc_tx_ready(struct atm_vcc *vcc, unsigned int size)
{
- struct sk_buff *skb;
struct sock *sk = sk_atm(vcc);
if (sk_wmem_alloc_get(sk) && !atm_may_send(vcc, size)) {
pr_debug("Sorry: wmem_alloc = %d, size = %d, sndbuf = %d\n",
sk_wmem_alloc_get(sk), size, sk->sk_sndbuf);
- return NULL;
+ return false;
}
- while (!(skb = alloc_skb(size, GFP_KERNEL)))
- schedule();
- pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
- atomic_add(skb->truesize, &sk->sk_wmem_alloc);
- return skb;
+ return true;
}
static void vcc_sock_destruct(struct sock *sk)
@@ -606,7 +601,7 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
eff = (size+3) & ~3; /* align to word boundary */
prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
error = 0;
- while (!(skb = alloc_tx(vcc, eff))) {
+ while (!vcc_tx_ready(vcc, eff)) {
if (m->msg_flags & MSG_DONTWAIT) {
error = -EAGAIN;
break;
@@ -628,6 +623,13 @@ int vcc_sendmsg(struct socket *sock, struct msghdr *m, size_t size)
finish_wait(sk_sleep(sk), &wait);
if (error)
goto out;
+
+ skb = alloc_skb(eff, GFP_KERNEL);
+ if (!skb)
+ goto out;
+ pr_debug("%d += %d\n", sk_wmem_alloc_get(sk), skb->truesize);
+ atomic_add(skb->truesize, &sk->sk_wmem_alloc);
+
skb->dev = NULL; /* for paths shared with net_device interfaces */
ATM_SKB(skb)->atm_options = vcc->atm_options;
if (!copy_from_iter_full(skb_put(skb, size), size, &m->msg_iter)) {
--
Ueimor
^ permalink raw reply related
* Re: [Patch net] atm: remove an unnecessary loop
From: Francois Romieu @ 2017-01-14 0:15 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Michal Hocko, Chas Williams,
Andrey Konovalov
In-Reply-To: <CAM_iQpUCVh9y8uhZ8Hj0mcjgfMGUQDvaEvSu5Wur1oVn+EZNGQ@mail.gmail.com>
Cong Wang <xiyou.wangcong@gmail.com> :
> On Fri, Jan 13, 2017 at 5:23 AM, Francois Romieu <romieu@fr.zoreil.com> wrote:
[...]
> > alloc_skb() does not need to be in the "while" loop.
>
> This is exactly what I describe in my changelog, don't know
> why you want to repeat it...
Because it is still hidden in a while loop.
You turned the alloc from a two level deep "while" loop to a one level
one. I want it at zero level. alloc_skb(..., GFP_KERNEL) fails ?
So let it be done (see patch in other message).
[...]
> Please don't expect me to fix many things in one patch, let's
> fix each of them separately, agreed?
I am not convinced that several patches are needed to get the whole
picture right.
--
Ueimor
^ 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