* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-20 17:38 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, jiri, netdev, xiyou.wangcong
In-Reply-To: <20170420.115000.250335491542942036.davem@davemloft.net>
On 17-04-20 11:50 AM, David Miller wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> Date: Thu, 20 Apr 2017 09:27:00 -0400
>
>> The issue Jiri is bringing up is unrelated. He is talking about
>> a bitmap and conflating it with a data structure. They are not
>> the same issue.
>
> Bitmaps can have the same exact problem as padding if we didn't code
> it correctly.
>
> The issue is _purely_, "did we check unused 'fields' and enforce them
> to be a certain value"
>
> If not, we lose, and can't use those "fields" in the future.
>
> This rule applies whether you are speaking about padding or a bitmask.
>
There are no examples of such issues with bitmasks encapsulated in TLVs
that exist.
I grep iproute2 code and there are tons of example of bitmask flags
being sent in TLVs. They all start with:
u64/32/16 mybitflags = 0;
if i want foo then
mybitflags |= BRIDGE_FLAGS_SELF;
if i want bar then
mybitflags |= xxxx
addattr16/32/64(&req.n, sizeof(req), ATTR_XXX, mybitflags);
It does not make much sense to have a TLV for each of these
bits when i can fit a bunch of them in u16/32/64.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH v6 3/4] net: dsa: LAN9303: add I2C managed mode support
From: Florian Fainelli @ 2017-04-20 17:38 UTC (permalink / raw)
To: Juergen Borleis, netdev-u79uwXL29TY76Z2rM5mHXA
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, andrew-g2DYL2Zd6BY,
vivien.didelot-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, devicetree-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
In-Reply-To: <20170418084827.19613-4-jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On 04/18/2017 01:48 AM, Juergen Borleis wrote:
> In this mode the switch device and the internal phys will be managed via
> I2C interface. The MDIO interface is still supported, but for the
> (emulated) CPU port only.
>
> Signed-off-by: Juergen Borleis <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> CC: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> CC: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> CC: mark.rutland-5wv7dgnIgG8@public.gmane.org
> ---
> .../devicetree/bindings/net/dsa/lan9303.txt | 62 +++++++++++
> drivers/net/dsa/Kconfig | 16 +++
> drivers/net/dsa/Makefile | 2 +
> drivers/net/dsa/lan9303_i2c.c | 113 +++++++++++++++++++++
> 4 files changed, 193 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/dsa/lan9303.txt
> create mode 100644 drivers/net/dsa/lan9303_i2c.c
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
> new file mode 100644
> index 0000000000000..2edc2561467a7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
> @@ -0,0 +1,62 @@
> +SMSC/MicroChip LAN9303 three port ethernet switch
> +-------------------------------------------------
> +
> +Required properties:
> +
> +- compatible: should be "smsc,lan9303-i2c"
> +
> +Optional properties:
> +
> +- reset-gpios: GPIO to be used to reset the whole device
> +- reset-duration: reset duration in milliseconds, defaults to 200 ms
> +
> +Subnodes:
> +
> +The integrated switch subnode should be specified according to the binding
> +described in dsa/dsa.txt. The CPU port of this switch is always port 0.
> +
> +Note: always use 'reg = <0/1/2>;' for the three DSA ports, even if the device is
> +configured to use 1/2/3 instead. This hardware configuration will be
> +auto-detected and mapped accordingly.
> +
> +Example:
> +
> +I2C managed mode:
> +
> + master: masterdevice@X {
> + status = "okay";
> +
> + fixed-link { /* RMII fixed link to LAN9303 */
> + speed = <100>;
> + full-duplex;
> + };
> + };
> +
> + switch: switch@a {
> + compatible = "smsc,lan9303-i2c";
> + reg = <0xa>;
> + status = "okay";
> + reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
> + reset-duration = <200>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 { /* RMII fixed link to master */
> + reg = <0>;
> + label = "cpu";
> + ethernet = <&master>;
> + };
> +
> + port@1 { /* external port 1 */
> + reg = <1>;
> + label = "lan1;
> + };
> +
> + port@2 { /* external port 2 */
> + reg = <2>;
> + label = "lan2";
> + };
> + };
> + };
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 31a2b229106dd..c56533bffc8ce 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -50,4 +50,20 @@ config NET_DSA_MT7530
> This enables support for the Mediatek MT7530 Ethernet switch
> chip.
>
> +config NET_DSA_SMSC_LAN9303
> + tristate
> + select NET_DSA_TAG_LAN9303
> + ---help---
> + This enables support for the SMSC/Microchip LAN9303 3 port ethernet
> + switch chips.
> +
Should not this hunk ^^^ be part of the previous patch (patch #2)? It's
not a big deal, but it would have been more natural and would allow
building the driver right away even in the absence of bus "glue" modules.
> +config NET_DSA_SMSC_LAN9303_I2C
> + tristate "SMSC/Microchip LAN9303 3-ports 10/100 ethernet switch in I2C managed mode"
> + depends on NET_DSA
> + select NET_DSA_SMSC_LAN9303
> + select REGMAP_I2C
> + ---help---
> + Enable access functions if the SMSC/Microchip LAN9303 is configured
> + for I2C managed mode.
> +
> endmenu
> diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
> index 2ae07f4fbf635..c1981ba18963f 100644
> --- a/drivers/net/dsa/Makefile
> +++ b/drivers/net/dsa/Makefile
> @@ -3,6 +3,8 @@ obj-$(CONFIG_NET_DSA_BCM_SF2) += bcm-sf2.o
> bcm-sf2-objs := bcm_sf2.o bcm_sf2_cfp.o
> obj-$(CONFIG_NET_DSA_QCA8K) += qca8k.o
> obj-$(CONFIG_NET_DSA_MT7530) += mt7530.o
> +obj-$(CONFIG_NET_DSA_SMSC_LAN9303) += lan9303-core.o
Same thing here.
Other than that:
Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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-next v5 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-20 17:39 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, jiri, xiyou.wangcong, netdev
In-Reply-To: <1492704598.22296.43.camel@edumazet-glaptop3.roam.corp.google.com>
On 17-04-20 12:09 PM, Eric Dumazet wrote:
> On Thu, 2017-04-20 at 09:06 -0400, Jamal Hadi Salim wrote:
>
>> nest = nla_nest_start(skb, TCA_ACT_TAB);
>> if (nest == NULL)
>> @@ -1113,6 +1136,8 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
>> if (ret > 0) {
>> nla_nest_end(skb, nest);
>> ret = skb->len;
>> + memcpy(nla_data(count_attr), &cb->args[1], sizeof(u32));
>
> This will not work on BigEndian 64bit hosts, since cb->args[1] is bigger
> than 32bit.
>
Ok, thanks.
I will assign to a 32 bit var first then memcpy in the next iteration
(tomorrow).
cheers,
jamal
^ permalink raw reply
* Re: [PATCH v6 2/4] net: dsa: add new DSA switch driver for the SMSC-LAN9303
From: Florian Fainelli @ 2017-04-20 17:41 UTC (permalink / raw)
To: Juergen Borleis, netdev
Cc: linux-kernel, kernel, andrew, vivien.didelot, davem
In-Reply-To: <20170418084827.19613-3-jbe@pengutronix.de>
On 04/18/2017 01:48 AM, Juergen Borleis wrote:
> The SMSC/Microchip LAN9303 is an ethernet switch device with one CPU port
> and two external ethernet ports with built-in phys.
>
> This driver uses the DSA framework, but is currently only capable of
> separating the two external ports. There is no offload support yet.
>
> Signed-off-by: Juergen Borleis <jbe@pengutronix.de>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCHv6] net: dsa: add SMSC/Microchip LAN9303 three port ethernet switch driver
From: David Miller @ 2017-04-20 17:50 UTC (permalink / raw)
To: jbe; +Cc: netdev, linux-kernel, f.fainelli, kernel, andrew, vivien.didelot
In-Reply-To: <20170418084827.19613-1-jbe@pengutronix.de>
From: Juergen Borleis <jbe@pengutronix.de>
Date: Tue, 18 Apr 2017 10:48:23 +0200
> The LAN9303 is a three port 10/100 ethernet switch with integrated phys
> for the two external ethernet ports. The third port is an RMII/MII
> interface to a host master network interface (e.g. fixed link).
>
> While the LAN9303 device itself supports offload packet processing, this
> driver does not make use of it yet. This driver just configures the device
> to provide two separate network interfaces (which is the default state of
> a DSA device).
>
> Please note: the "MDIO managed mode" driver part isn't tested yet. I have
> used and tested the "I2C managed mode" only.
>
> Changes in v6:
...
Series applied, thanks.
^ permalink raw reply
* Re: pull-request: mac80211-next 2017-04-18
From: David Miller @ 2017-04-20 17:55 UTC (permalink / raw)
To: johannes; +Cc: netdev, linux-wireless
In-Reply-To: <20170418092030.7299-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 18 Apr 2017 11:20:29 +0200
> I hadn't realized that I actually had this many patches pending,
> but most of them really are cleanups and little fixes. Despite a
> bunch of driver changes, there don't seem to be any conflicts.
>
> Please pull and let me know if there's any problem.
Pulled, thanks Johannes.
^ permalink raw reply
* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: David Miller @ 2017-04-20 17:58 UTC (permalink / raw)
To: jhs; +Cc: eric.dumazet, jiri, netdev, xiyou.wangcong
In-Reply-To: <95110642-0f9c-d622-3ca5-16ad5115da2f@mojatatu.com>
From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Thu, 20 Apr 2017 13:38:14 -0400
> On 17-04-20 11:50 AM, David Miller wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>> Date: Thu, 20 Apr 2017 09:27:00 -0400
>>
>>> The issue Jiri is bringing up is unrelated. He is talking about
>>> a bitmap and conflating it with a data structure. They are not
>>> the same issue.
>>
>> Bitmaps can have the same exact problem as padding if we didn't code
>> it correctly.
>>
>> The issue is _purely_, "did we check unused 'fields' and enforce them
>> to be a certain value"
>>
>> If not, we lose, and can't use those "fields" in the future.
>>
>> This rule applies whether you are speaking about padding or a bitmask.
>>
>
> There are no examples of such issues with bitmasks encapsulated in
> TLVs
> that exist.
> I grep iproute2 code and there are tons of example of bitmask flags
> being sent in TLVs. They all start with:
>
> u64/32/16 mybitflags = 0;
>
> if i want foo then
> mybitflags |= BRIDGE_FLAGS_SELF;
> if i want bar then
> mybitflags |= xxxx
>
> addattr16/32/64(&req.n, sizeof(req), ATTR_XXX, mybitflags);
>
> It does not make much sense to have a TLV for each of these
> bits when i can fit a bunch of them in u16/32/64.
I have not ruled out bitmasks. I'm only saying that the kernel must
properly reject bits it doesn't recognize when they are set.
Each bit must have a strict semantic, even unused ones, otherwise
unused ones may never safely be used in the future.
^ permalink raw reply
* more on FP operations
From: David Miller @ 2017-04-20 18:06 UTC (permalink / raw)
To: ast; +Cc: borkmann, netdev
I'm running test_verifier for testing, and I notice in my JIT that a
32-bit move from the frame pointer (BPF_REG_10) ends up in the JIT.
It is from this test:
"unpriv: partial copy of pointer",
.insns = {
BPF_MOV32_REG(BPF_REG_1, BPF_REG_10),
BPF_MOV64_IMM(BPF_REG_0, 0),
BPF_EXIT_INSN(),
},
.errstr_unpriv = "R10 partial copy",
.result_unpriv = REJECT,
.result = ACCEPT,
It seems to suggest that privileged code is allowed to do this, but I
can't think of a legitimate usage.
I really want to be able to JIT anything the verifier accepts, but I
have a hard time justifying adding 32-bit FP register move support,
adjusting for the stack bias, etc.
Thanks.
^ permalink raw reply
* Re: [PATCH next] bonding: fix wq initialization for links created via netlink
From: Joe Stringer @ 2017-04-20 18:06 UTC (permalink / raw)
To: Mahesh Bandewar
Cc: Jay Vosburgh, Andy Gospodarek, Veaceslav Falico,
Nikolay Aleksandrov, David Miller, Eric Dumazet, netdev,
Mahesh Bandewar
In-Reply-To: <20170420003042.18045-1-mahesh@bandewar.net>
On 19 April 2017 at 17:30, Mahesh Bandewar <mahesh@bandewar.net> wrote:
> From: Mahesh Bandewar <maheshb@google.com>
>
> Earlier patch 4493b81bea ("bonding: initialize work-queues during
> creation of bond") moved the work-queue initialization from bond_open()
> to bond_create(). However this caused the link those are created using
> netlink 'create bond option' (ip link add bondX type bond); create the
> new trunk without initializing work-queues. Prior to the above mentioned
> change, ndo_open was in both paths and things worked correctly. The
> consequence is visible in the report shared by Joe Stringer -
>
> I've noticed that this patch breaks bonding within namespaces if
> you're not careful to perform device cleanup correctly.
>
> Here's my repro script, you can run on any net-next with this patch
> and you'll start seeing some weird behaviour:
>
> ip netns add foo
> ip li add veth0 type veth peer name veth0+ netns foo
> ip li add veth1 type veth peer name veth1+ netns foo
> ip netns exec foo ip li add bond0 type bond
> ip netns exec foo ip li set dev veth0+ master bond0
> ip netns exec foo ip li set dev veth1+ master bond0
> ip netns exec foo ip addr add dev bond0 192.168.0.1/24
> ip netns exec foo ip li set dev bond0 up
> ip li del dev veth0
> ip li del dev veth1
>
> The second to last command segfaults, last command hangs. rtnl is now
> permanently locked. It's not a problem if you take bond0 down before
> deleting veths, or delete bond0 before deleting veths. If you delete
> either end of the veth pair as per above, either inside or outside the
> namespace, it hits this problem.
>
> Here's some kernel logs:
> [ 1221.801610] bond0: Enslaving veth0+ as an active interface with an up link
> [ 1224.449581] bond0: Enslaving veth1+ as an active interface with an up link
> [ 1281.193863] bond0: Releasing backup interface veth0+
> [ 1281.193866] bond0: the permanent HWaddr of veth0+ -
> 16:bf:fb:e0:b8:43 - is still in use by bond0 - set the HWaddr of
> veth0+ to a different address to avoid conflicts
> [ 1281.193867] ------------[ cut here ]------------
> [ 1281.193873] WARNING: CPU: 0 PID: 2024 at kernel/workqueue.c:1511
> __queue_delayed_work+0x13f/0x150
> [ 1281.193873] Modules linked in: bonding veth openvswitch nf_nat_ipv6
> nf_nat_ipv4 nf_nat autofs4 nfsd auth_rpcgss nfs_acl binfmt_misc nfs
> lockd grace sunrpc fscache ppdev vmw_balloon coretemp psmouse
> serio_raw vmwgfx ttm drm_kms_helper vmw_vmci netconsole parport_pc
> configfs drm i2c_piix4 fb_sys_fops syscopyarea sysfillrect sysimgblt
> shpchp mac_hid nf_conntrack_ipv6 nf_defrag_ipv6 nf_conntrack_ipv4
> nf_defrag_ipv4 nf_conntrack libcrc32c lp parport hid_generic usbhid
> hid mptspi mptscsih e1000 mptbase ahci libahci
> [ 1281.193905] CPU: 0 PID: 2024 Comm: ip Tainted: G W
> 4.10.0-bisect-bond-v0.14 #37
> [ 1281.193906] Hardware name: VMware, Inc. VMware Virtual
> Platform/440BX Desktop Reference Platform, BIOS 6.00 09/30/2014
> [ 1281.193906] Call Trace:
> [ 1281.193912] dump_stack+0x63/0x89
> [ 1281.193915] __warn+0xd1/0xf0
> [ 1281.193917] warn_slowpath_null+0x1d/0x20
> [ 1281.193918] __queue_delayed_work+0x13f/0x150
> [ 1281.193920] queue_delayed_work_on+0x27/0x40
> [ 1281.193929] bond_change_active_slave+0x25b/0x670 [bonding]
> [ 1281.193932] ? synchronize_rcu_expedited+0x27/0x30
> [ 1281.193935] __bond_release_one+0x489/0x510 [bonding]
> [ 1281.193939] ? addrconf_notify+0x1b7/0xab0
> [ 1281.193942] bond_netdev_event+0x2c5/0x2e0 [bonding]
> [ 1281.193944] ? netconsole_netdev_event+0x124/0x190 [netconsole]
> [ 1281.193947] notifier_call_chain+0x49/0x70
> [ 1281.193948] raw_notifier_call_chain+0x16/0x20
> [ 1281.193950] call_netdevice_notifiers_info+0x35/0x60
> [ 1281.193951] rollback_registered_many+0x23b/0x3e0
> [ 1281.193953] unregister_netdevice_many+0x24/0xd0
> [ 1281.193955] rtnl_delete_link+0x3c/0x50
> [ 1281.193956] rtnl_dellink+0x8d/0x1b0
> [ 1281.193960] rtnetlink_rcv_msg+0x95/0x220
> [ 1281.193962] ? __kmalloc_node_track_caller+0x35/0x280
> [ 1281.193964] ? __netlink_lookup+0xf1/0x110
> [ 1281.193966] ? rtnl_newlink+0x830/0x830
> [ 1281.193967] netlink_rcv_skb+0xa7/0xc0
> [ 1281.193969] rtnetlink_rcv+0x28/0x30
> [ 1281.193970] netlink_unicast+0x15b/0x210
> [ 1281.193971] netlink_sendmsg+0x319/0x390
> [ 1281.193974] sock_sendmsg+0x38/0x50
> [ 1281.193975] ___sys_sendmsg+0x25c/0x270
> [ 1281.193978] ? mem_cgroup_commit_charge+0x76/0xf0
> [ 1281.193981] ? page_add_new_anon_rmap+0x89/0xc0
> [ 1281.193984] ? lru_cache_add_active_or_unevictable+0x35/0xb0
> [ 1281.193985] ? __handle_mm_fault+0x4e9/0x1170
> [ 1281.193987] __sys_sendmsg+0x45/0x80
> [ 1281.193989] SyS_sendmsg+0x12/0x20
> [ 1281.193991] do_syscall_64+0x6e/0x180
> [ 1281.193993] entry_SYSCALL64_slow_path+0x25/0x25
> [ 1281.193995] RIP: 0033:0x7f6ec122f5a0
> [ 1281.193995] RSP: 002b:00007ffe69e89c48 EFLAGS: 00000246 ORIG_RAX:
> 000000000000002e
> [ 1281.193997] RAX: ffffffffffffffda RBX: 00007ffe69e8dd60 RCX: 00007f6ec122f5a0
> [ 1281.193997] RDX: 0000000000000000 RSI: 00007ffe69e89c90 RDI: 0000000000000003
> [ 1281.193998] RBP: 00007ffe69e89c90 R08: 0000000000000000 R09: 0000000000000003
> [ 1281.193999] R10: 00007ffe69e89a10 R11: 0000000000000246 R12: 0000000058f14b9f
> [ 1281.193999] R13: 0000000000000000 R14: 00000000006473a0 R15: 00007ffe69e8e450
> [ 1281.194001] ---[ end trace 713a77486cbfbfa3 ]---
>
> Fixes: 4493b81bea ("bonding: initialize work-queues during creation of bond")
> Reported-by: Joe Stringer <joe@ovn.org>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
Thanks Mahesh, this fixes the issue I was observing.
Tested-by: Joe Stringer <joe@ovn.org>
> drivers/net/bonding/bond_main.c | 2 +-
> drivers/net/bonding/bond_netlink.c | 5 +++++
> include/net/bonding.h | 1 +
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 6bd3b50faf48..e549bf6f5cac 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3243,7 +3243,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>
> /*-------------------------- Device entry points ----------------------------*/
>
> -static void bond_work_init_all(struct bonding *bond)
> +void bond_work_init_all(struct bonding *bond)
> {
> INIT_DELAYED_WORK(&bond->mcast_work,
> bond_resend_igmp_join_requests_delayed);
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index b8df0f5e8c25..31b9f37c7dbd 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -449,6 +449,11 @@ static int bond_newlink(struct net *src_net, struct net_device *bond_dev,
> err = register_netdevice(bond_dev);
>
> netif_carrier_off(bond_dev);
> + if (err >= 0) {
register_netdevice() returns 0 on success according to its
documentation, does it need to handle positive err values?
> + struct bonding *bond = netdev_priv(bond_dev);
> +
> + bond_work_init_all(bond);
> + }
>
> return err;
> }
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 04a21e8048be..b00508d22e0a 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -614,6 +614,7 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
> int level);
> int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
> void bond_slave_arr_work_rearm(struct bonding *bond, unsigned long delay);
> +void bond_work_init_all(struct bonding *bond);
>
> #ifdef CONFIG_PROC_FS
> void bond_create_proc_entry(struct bonding *bond);
> --
> 2.12.2.816.g2cccc81164-goog
>
^ permalink raw reply
* Re: [PATCH net] netfilter: xt_TCPMSS: add more sanity tests on tcph->doff
From: Denys Fedoryshchenko @ 2017-04-20 18:14 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Eric Dumazet, Linux Kernel Network Developers, netfilter-devel
In-Reply-To: <20170408202443.GA3373@salvia>
On 2017-04-08 23:24, Pablo Neira Ayuso wrote:
> On Mon, Apr 03, 2017 at 10:55:11AM -0700, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> Denys provided an awesome KASAN report pointing to an use
>> after free in xt_TCPMSS
>>
>> I have provided three patches to fix this issue, either in xt_TCPMSS
>> or
>> in xt_tcpudp.c. It seems xt_TCPMSS patch has the smallest possible
>> impact.
>
> Applied to nf.git, thanks!
Any plans to queue it to stable trees?
It seems affected kernel for years.
^ permalink raw reply
* [PATCH] p54: Prevent from dereferencing null pointer when releasing SKB
From: Myungho Jung @ 2017-04-20 18:25 UTC (permalink / raw)
To: edumazet; +Cc: netdev
Added NULL check to make __dev_kfree_skb_irq consistent with kfree
family of functions.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289
Signed-off-by: Myungho Jung <mhjungk@gmail.com>
---
net/core/dev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index 7869ae3..22be2a6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2450,6 +2450,9 @@ void __dev_kfree_skb_irq(struct sk_buff *skb, enum skb_free_reason reason)
{
unsigned long flags;
+ if (unlikely(!skb))
+ return;
+
if (likely(atomic_read(&skb->users) == 1)) {
smp_rmb();
atomic_set(&skb->users, 0);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net] netdevice: Prefer NETIF_F_HW_CSUM when intersecting features
From: Alexander Duyck @ 2017-04-20 18:36 UTC (permalink / raw)
To: Vladislav Yasevich
Cc: Netdev, Tom Herbert, Michal Kubecek, Vladislav Yasevich
In-Reply-To: <1492650743-28164-1-git-send-email-vyasevic@redhat.com>
On Wed, Apr 19, 2017 at 6:12 PM, Vladislav Yasevich <vyasevich@gmail.com> wrote:
> While hardware device use either NETIF_F_(IP|IPV6)_CSUM or
> NETIF_F_HW_CSUM, all of the software devices use HW_CSUM.
> This results in an interesting situation when the software
> device is configured on top of hw device using (IP|IPV6)_CSUM.
> In this situation, the user can't turn off checksum offloading
> features on the software device.
>
> This patch resolves that by prefering the NETIF_F_HW_CSUM setting
> when computing a feature intersect.
>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
> include/linux/netdevice.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 97456b25..3d811c1 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4019,9 +4019,9 @@ static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
> {
> if ((f1 ^ f2) & NETIF_F_HW_CSUM) {
> if (f1 & NETIF_F_HW_CSUM)
> - f1 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
> + f2 |= NETIF_F_HW_CSUM;
> else
> - f2 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
> + f1 |= NETIF_F_HW_CSUM;
> }
>
> return f1 & f2;
> --
> 2.7.4
>
This doesn't seem right to me. I can see promoting NETIF_F_HW_CSUM to
also include IP_CSUM and IPV6_CSUM but all this seems to do is force
NETIF_F_HW_CSUM on always if either side has it enabled. That doesn't
seem like an intersection at all to me.
What this code is supposed to do is make it so that if we are testing
for NETIF_F_IP_CSUM and NETIF_F_HW_CSUM is set it will return true.
The code as it is now will always return true if we test for
NETIF_F_HW_CSUM since it will just set it on the other side for us.
- Alex
^ permalink raw reply
* Re: [PATCH rdma-next v2 00/12] Omni-Path Virtual Network Interface Controller (VNIC)
From: Doug Ledford @ 2017-04-20 18:39 UTC (permalink / raw)
To: Vishwanathapura, Niranjana
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w,
ira.weiny-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <1492054170-23577-1-git-send-email-niranjana.vishwanathapura-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
On Wed, 2017-04-12 at 20:29 -0700, Vishwanathapura, Niranjana wrote:
> ChangeLog:
> =========
> v1 => v2:
> a) Change error code for unsupported rdma netdev type
> b) Remove some debug messages
>
> v1: posted @ https://www.spinics.net/lists/linux-rdma/msg48518.html
>
> v0 => v1:
> a) changes as required by new kernel base (for-4.12)
> b) moved rdma netdev interface into a separate patch
> c) Return specific error code if specified rdma netdev type is not
> supported
> d) Some minor fixes (no changes to overall design or interface)
>
> v0: Initial post @ https://www.spinics.net/lists/linux-rdma/msg46604.
> html
>
> Description:
> ============
> Intel Omni-Path (OPA) Virtual Network Interface Controller (VNIC)
> feature
> supports Ethernet functionality over Omni-Path fabric by
> encapsulating
> the Ethernet packets between HFI nodes.
I've pulled this into a custom branch based on DaveM's for-next tree so
that I could put the IPoIB changes into the same branch and the final
touchup patch Leon talked about into that branch as well. So, this is
now queued for-next (albeit not on the normal branch).
--
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
--
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-next 00/16] Mellanox, mlx5 RDMA net device support
From: Doug Ledford @ 2017-04-20 18:41 UTC (permalink / raw)
To: David Miller, saeedm
Cc: netdev, linux-rdma, leonro, erezsh, jgunthorpe,
niranjana.vishwanathapura
In-Reply-To: <20170417.110851.1496380796999919507.davem@davemloft.net>
On Mon, 2017-04-17 at 11:08 -0400, David Miller wrote:
> From: Saeed Mahameed <saeedm@mellanox.com>
> Date: Thu, 13 Apr 2017 06:36:50 +0300
>
> > This series provides the lower level mlx5 support of RDMA netdevice
> > creation API [1] suggested and introduced by Intel's HFI OPA VNIC
> > netdevice driver [2], to enable IPoIB mlx5 RDMA netdevice creation.
>
> Series applied to net-next.
I've pulled Dave's net-next into a branch in my tree for the IPoIB
patches to go on top of so that way it can hold the final touchup patch
without any conflicts anywhere.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
^ permalink raw reply
* Re: [PATCH next] bonding: fix wq initialization for links created via netlink
From: Mahesh Bandewar (महेश बंडेवार) @ 2017-04-20 18:44 UTC (permalink / raw)
To: Joe Stringer
Cc: Mahesh Bandewar, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico,
Nikolay Aleksandrov, David Miller, Eric Dumazet, netdev
In-Reply-To: <CAPWQB7Hj-W9NPEUSBBCh6Av8ri0mScs+SWCc=mc=yCQkD3w90w@mail.gmail.com>
On Thu, Apr 20, 2017 at 11:06 AM, Joe Stringer <joe@ovn.org> wrote:
> On 19 April 2017 at 17:30, Mahesh Bandewar <mahesh@bandewar.net> wrote:
>> From: Mahesh Bandewar <maheshb@google.com>
>>
[...]
>>
>> Fixes: 4493b81bea ("bonding: initialize work-queues during creation of bond")
>> Reported-by: Joe Stringer <joe@ovn.org>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>> ---
>
> Thanks Mahesh, this fixes the issue I was observing.
>
> Tested-by: Joe Stringer <joe@ovn.org>
>
Thanks for the confirmation Joe.
>> drivers/net/bonding/bond_main.c | 2 +-
>> drivers/net/bonding/bond_netlink.c | 5 +++++
>> include/net/bonding.h | 1 +
>> 3 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 6bd3b50faf48..e549bf6f5cac 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -3243,7 +3243,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>>
>> /*-------------------------- Device entry points ----------------------------*/
>>
>> -static void bond_work_init_all(struct bonding *bond)
>> +void bond_work_init_all(struct bonding *bond)
>> {
>> INIT_DELAYED_WORK(&bond->mcast_work,
>> bond_resend_igmp_join_requests_delayed);
>> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
>> index b8df0f5e8c25..31b9f37c7dbd 100644
>> --- a/drivers/net/bonding/bond_netlink.c
>> +++ b/drivers/net/bonding/bond_netlink.c
>> @@ -449,6 +449,11 @@ static int bond_newlink(struct net *src_net, struct net_device *bond_dev,
>> err = register_netdevice(bond_dev);
>>
>> netif_carrier_off(bond_dev);
>> + if (err >= 0) {
>
> register_netdevice() returns 0 on success according to its
> documentation, does it need to handle positive err values?
>
Yes, that's not needed and could cause problems. Will update the patch.
>> + struct bonding *bond = netdev_priv(bond_dev);
>> +
>> + bond_work_init_all(bond);
>> + }
>>
>> return err;
>> }
>> diff --git a/include/net/bonding.h b/include/net/bonding.h
>> index 04a21e8048be..b00508d22e0a 100644
>> --- a/include/net/bonding.h
>> +++ b/include/net/bonding.h
>> @@ -614,6 +614,7 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
>> int level);
>> int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
>> void bond_slave_arr_work_rearm(struct bonding *bond, unsigned long delay);
>> +void bond_work_init_all(struct bonding *bond);
>>
>> #ifdef CONFIG_PROC_FS
>> void bond_create_proc_entry(struct bonding *bond);
>> --
>> 2.12.2.816.g2cccc81164-goog
>>
^ permalink raw reply
* Re: [PATCH net-next 00/16] Mellanox, mlx5 RDMA net device support
From: Leon Romanovsky @ 2017-04-20 18:49 UTC (permalink / raw)
To: Doug Ledford
Cc: David Miller, saeedm-VPRAkNaXOzVWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
erezsh-VPRAkNaXOzVWk0Htik3J/w,
jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/,
niranjana.vishwanathapura-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <1492713677.3041.3.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]
On Thu, Apr 20, 2017 at 02:41:17PM -0400, Doug Ledford wrote:
> On Mon, 2017-04-17 at 11:08 -0400, David Miller wrote:
> > From: Saeed Mahameed <saeedm-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Date: Thu, 13 Apr 2017 06:36:50 +0300
> >
> > > This series provides the lower level mlx5 support of RDMA netdevice
> > > creation API [1] suggested and introduced by Intel's HFI OPA VNIC
> > > netdevice driver [2], to enable IPoIB mlx5 RDMA netdevice creation.
> >
> > Series applied to net-next.
>
> I've pulled Dave's net-next into a branch in my tree for the IPoIB
> patches to go on top of so that way it can hold the final touchup patch
> without any conflicts anywhere.
Thanks,
How and when do you want us to forward it?
Thanks
>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> GPG KeyID: B826A3330E572FDD
>
> Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
>
> --
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] openvswitch: Add eventmask support to CT action.
From: Joe Stringer @ 2017-04-20 18:53 UTC (permalink / raw)
To: Jarno Rajahalme; +Cc: netdev
In-Reply-To: <1492652976-99201-2-git-send-email-jarno@ovn.org>
On 19 April 2017 at 18:49, Jarno Rajahalme <jarno@ovn.org> wrote:
> Add a new optional conntrack action attribute OVS_CT_ATTR_EVENTMASK,
> which can be used in conjunction with the commit flag
> (OVS_CT_ATTR_COMMIT) to set the mask of bits specifying which
> conntrack events (IPCT_*) should be delivered via the Netfilter
> netlink multicast groups. Default behavior depends on the system
> configuration, but typically a lot of events are delivered. This can be
> very chatty for the NFNLGRP_CONNTRACK_UPDATE group, even if only some
> types of events are of interest.
>
> Netfilter core init_conntrack() adds the event cache extension, so we
> only need to set the ctmask value. However, if the system is
> configured without support for events, the setting will be skipped due
> to extension not being found.
>
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
> ---
Thanks Jarno, looks good overall. Minor nit in the API description below.
Acked-by: Joe Stringer <joe@ovn.org>
> include/uapi/linux/openvswitch.h | 12 ++++++++++++
> net/openvswitch/conntrack.c | 27 +++++++++++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index 66d1c3c..38ae95d 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -693,6 +693,17 @@ struct ovs_action_hash {
> * nothing if the connection is already committed will check that the current
> * packet is in conntrack entry's original direction. If directionality does
> * not match, will delete the existing conntrack entry and commit a new one.
> + * @OVS_CT_ATTR_EVENTMASK: Mask of bits indicating which conntrack event types
> + * (enum ip_conntrack_events IPCT_*) should be reported. For any bit set to
> + * zero, the corresponding event type is not generated. Default behavior
> + * depends on system configuration, but typically all event types are
> + * generated, hence listening on UPDATE events may get a lot of events.
s/UPDATE/NFNLGRP_CONNTRACK_UPDATE/
> + * Explicitly passing this attribute allows limiting the updates received to
> + * the events of interest. The bit 1 << IPCT_NEW, 1 << IPCT_RELATED, and
> + * 1 << IPCT_DESTROY must be set to ones for those events to be received on
> + * NFNLGRP_CONNTRACK_NEW and NFNLGRP_CONNTRACK_DESTROY groups, respectively.
> + * Remaining bits control the changes for which an event is delivered on the
> + * NFNLGRP_CONNTRACK_UPDATE group.
> */
> enum ovs_ct_attr {
> OVS_CT_ATTR_UNSPEC,
> @@ -704,6 +715,7 @@ enum ovs_ct_attr {
> related connections. */
> OVS_CT_ATTR_NAT, /* Nested OVS_NAT_ATTR_* */
> OVS_CT_ATTR_FORCE_COMMIT, /* No argument */
> + OVS_CT_ATTR_EVENTMASK, /* u32 mask of IPCT_* events. */
> __OVS_CT_ATTR_MAX
> };
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 58de4c2..4f7c3b5 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -66,7 +66,9 @@ struct ovs_conntrack_info {
> u8 commit : 1;
> u8 nat : 3; /* enum ovs_ct_nat */
> u8 force : 1;
> + u8 have_eventmask : 1;
> u16 family;
> + u32 eventmask; /* Mask of 1 << IPCT_*. */
> struct md_mark mark;
> struct md_labels labels;
> #ifdef CONFIG_NF_NAT_NEEDED
> @@ -1007,6 +1009,20 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
> if (!ct)
> return 0;
>
> + /* Set the conntrack event mask if given. NEW and DELETE events have
> + * their own groups, but the NFNLGRP_CONNTRACK_UPDATE group listener
> + * typically would receive many kinds of updates. Setting the event
> + * mask allows those events to be filtered. The set event mask will
> + * remain in effect for the lifetime of the connection unless changed
> + * by a further CT action with both the commit flag and the eventmask
> + * option. */
> + if (info->have_eventmask) {
> + struct nf_conntrack_ecache *cache = nf_ct_ecache_find(ct);
> +
> + if (cache)
> + cache->ctmask = info->eventmask;
> + }
> +
> /* Apply changes before confirming the connection so that the initial
> * conntrack NEW netlink event carries the values given in the CT
> * action.
> @@ -1238,6 +1254,8 @@ static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
> /* NAT length is checked when parsing the nested attributes. */
> [OVS_CT_ATTR_NAT] = { .minlen = 0, .maxlen = INT_MAX },
> #endif
> + [OVS_CT_ATTR_EVENTMASK] = { .minlen = sizeof(u32),
> + .maxlen = sizeof(u32) },
> };
>
> static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
> @@ -1316,6 +1334,11 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
> break;
> }
> #endif
> + case OVS_CT_ATTR_EVENTMASK:
> + info->have_eventmask = true;
> + info->eventmask = nla_get_u32(a);
> + break;
> +
> default:
> OVS_NLERR(log, "Unknown conntrack attr (%d)",
> type);
> @@ -1515,6 +1538,10 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
> ct_info->helper->name))
> return -EMSGSIZE;
> }
> + if (ct_info->have_eventmask &&
> + nla_put_u32(skb, OVS_CT_ATTR_EVENTMASK, ct_info->eventmask))
> + return -EMSGSIZE;
> +
> #ifdef CONFIG_NF_NAT_NEEDED
> if (ct_info->nat && !ovs_ct_nat_to_attr(ct_info, skb))
> return -EMSGSIZE;
> --
> 2.1.4
>
^ permalink raw reply
* Re: [PATCH net] netdevice: Prefer NETIF_F_HW_CSUM when intersecting features
From: Vlad Yasevich @ 2017-04-20 19:07 UTC (permalink / raw)
To: Alexander Duyck, Vladislav Yasevich; +Cc: Netdev, Tom Herbert, Michal Kubecek
In-Reply-To: <CAKgT0Ue67jdAcFeyXqv4Nr8+A-oKNBk7+xV_WmcfWdrNRgRGTQ@mail.gmail.com>
On 04/20/2017 02:36 PM, Alexander Duyck wrote:
> On Wed, Apr 19, 2017 at 6:12 PM, Vladislav Yasevich <vyasevich@gmail.com> wrote:
>> While hardware device use either NETIF_F_(IP|IPV6)_CSUM or
>> NETIF_F_HW_CSUM, all of the software devices use HW_CSUM.
>> This results in an interesting situation when the software
>> device is configured on top of hw device using (IP|IPV6)_CSUM.
>> In this situation, the user can't turn off checksum offloading
>> features on the software device.
>>
>> This patch resolves that by prefering the NETIF_F_HW_CSUM setting
>> when computing a feature intersect.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>> include/linux/netdevice.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 97456b25..3d811c1 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -4019,9 +4019,9 @@ static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
>> {
>> if ((f1 ^ f2) & NETIF_F_HW_CSUM) {
>> if (f1 & NETIF_F_HW_CSUM)
>> - f1 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
>> + f2 |= NETIF_F_HW_CSUM;
>> else
>> - f2 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
>> + f1 |= NETIF_F_HW_CSUM;
>> }
>>
>> return f1 & f2;
>> --
>> 2.7.4
>>
>
> This doesn't seem right to me. I can see promoting NETIF_F_HW_CSUM to
> also include IP_CSUM and IPV6_CSUM but all this seems to do is force
> NETIF_F_HW_CSUM on always if either side has it enabled. That doesn't
> seem like an intersection at all to me.
I think it needs to be the other way around. At least that's what I've
been observing...
>
> What this code is supposed to do is make it so that if we are testing
> for NETIF_F_IP_CSUM and NETIF_F_HW_CSUM is set it will return true.
> The code as it is now will always return true if we test for
> NETIF_F_HW_CSUM since it will just set it on the other side for us.
>
Yes, you are right. The way the code is now, if we turn off checksum
offload on the lower device, the upper device doesn't change.
What we really want to do is add HW_CSUM to the mask if IP|IPV6 is set.
That way, if lower device has IP|IPV6, it will end up with IP|IPV6|HW.
This way, controlling IP csums on the lower device will turn off the
checksum on the upper, while at the same time give us the ability to
control the checksum offload on the upper device itself.
Thanks. I'll resubmit a v2.
-vlad
^ permalink raw reply
* Re: [PATCH] netpoll: Check for skb->queue_mapping
From: tndave @ 2017-04-20 19:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, edumazet, brouer, netdev, sowmini.varadhan
In-Reply-To: <b98cb4f6-2e4c-71ba-1431-1824bf334f8e@oracle.com>
On 04/12/2017 03:37 PM, tndave wrote:
>
>
> On 04/06/2017 12:14 PM, Eric Dumazet wrote:
>> On Thu, 2017-04-06 at 12:07 -0700, tndave wrote:
>>
>>>> + q_index = q_index % dev->real_num_tx_queues;
>>> cpu interrupted here and dev->real_num_tx_queues has reduced!
>>>> + skb_set_queue_mapping(skb, q_index);
>>>> + }
>>>> + txq = netdev_get_tx_queue(dev, q_index);
>>> or cpu interrupted here and dev->real_num_tx_queues has reduced!
>>
>> If dev->real_num_tx_queues can be changed while this code is running we
>> are in deep deep trouble.
>>
>> Better make sure that when control path does this change, device (and/pr
>> netpoll) is frozen and no packet can be sent.
> When control path is making change to real_num_tx_queues, underlying
> device is disabled; also netdev tx queues are stopped/disabled so
> certainly no transmit is happening.
>
>
> The corner case I was referring is if netpoll's queue_process() code is
> interrupted and while it is not running, control path makes change to
> dev->real_num_tx_queues and exits. Later on, interrupted queue_process()
> resume execution and it can end up with wrong skb->queue_mapping and txq.
> We can prevent this case with below change:
>
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 9424673..29be246 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -105,15 +105,21 @@ static void queue_process(struct work_struct *work)
> while ((skb = skb_dequeue(&npinfo->txq))) {
> struct net_device *dev = skb->dev;
> struct netdev_queue *txq;
> + unsigned int q_index;
>
> if (!netif_device_present(dev) || !netif_running(dev)) {
> kfree_skb(skb);
> continue;
> }
>
> - txq = skb_get_tx_queue(dev, skb);
> -
> local_irq_save(flags);
> + /* check if skb->queue_mapping is still valid */
> + q_index = skb_get_queue_mapping(skb);
> + if (unlikely(q_index >= dev->real_num_tx_queues)) {
> + q_index = q_index % dev->real_num_tx_queues;
> + skb_set_queue_mapping(skb, q_index);
> + }
> + txq = netdev_get_tx_queue(dev, q_index);
> HARD_TX_LOCK(dev, txq, smp_processor_id());
> if (netif_xmit_frozen_or_stopped(txq) ||
> netpoll_start_xmit(skb, dev, txq) != NETDEV_TX_OK) {
>
>
> Thanks for your patience.
Eric,
Let me know if you are okay with above changes and me sending v2.
-Tushar
>
> -Tushar
>>
>>
>>
>>
>
^ permalink raw reply
* Re: [PATCH] netpoll: Check for skb->queue_mapping
From: Eric Dumazet @ 2017-04-20 19:13 UTC (permalink / raw)
To: tndave
Cc: Eric Dumazet, David Miller, Jesper Dangaard Brouer, netdev,
Sowmini Varadhan
In-Reply-To: <a94139a3-9eaf-6682-3dff-cfb161e62d32@oracle.com>
On Thu, Apr 20, 2017 at 12:08 PM, tndave <tushar.n.dave@oracle.com> wrote:
>
>
> On 04/12/2017 03:37 PM, tndave wrote:
>>
>>
>>
>> On 04/06/2017 12:14 PM, Eric Dumazet wrote:
>>>
>>> On Thu, 2017-04-06 at 12:07 -0700, tndave wrote:
>>>
>>>>> + q_index = q_index % dev->real_num_tx_queues;
>>>>
>>>> cpu interrupted here and dev->real_num_tx_queues has reduced!
>>>>>
>>>>> + skb_set_queue_mapping(skb, q_index);
>>>>> + }
>>>>> + txq = netdev_get_tx_queue(dev, q_index);
>>>>
>>>> or cpu interrupted here and dev->real_num_tx_queues has reduced!
>>>
>>>
>>> If dev->real_num_tx_queues can be changed while this code is running we
>>> are in deep deep trouble.
>>>
>>> Better make sure that when control path does this change, device (and/pr
>>> netpoll) is frozen and no packet can be sent.
>>
>> When control path is making change to real_num_tx_queues, underlying
>> device is disabled; also netdev tx queues are stopped/disabled so
>> certainly no transmit is happening.
>>
>>
>> The corner case I was referring is if netpoll's queue_process() code is
>> interrupted and while it is not running, control path makes change to
>> dev->real_num_tx_queues and exits. Later on, interrupted queue_process()
>> resume execution and it can end up with wrong skb->queue_mapping and txq.
>> We can prevent this case with below change:
>>
>> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
>> index 9424673..29be246 100644
>> --- a/net/core/netpoll.c
>> +++ b/net/core/netpoll.c
>> @@ -105,15 +105,21 @@ static void queue_process(struct work_struct *work)
>> while ((skb = skb_dequeue(&npinfo->txq))) {
>> struct net_device *dev = skb->dev;
>> struct netdev_queue *txq;
>> + unsigned int q_index;
>>
>> if (!netif_device_present(dev) || !netif_running(dev)) {
>> kfree_skb(skb);
>> continue;
>> }
>>
>> - txq = skb_get_tx_queue(dev, skb);
>> -
>> local_irq_save(flags);
>> + /* check if skb->queue_mapping is still valid */
>> + q_index = skb_get_queue_mapping(skb);
>> + if (unlikely(q_index >= dev->real_num_tx_queues)) {
>> + q_index = q_index % dev->real_num_tx_queues;
>> + skb_set_queue_mapping(skb, q_index);
>> + }
>> + txq = netdev_get_tx_queue(dev, q_index);
>> HARD_TX_LOCK(dev, txq, smp_processor_id());
>> if (netif_xmit_frozen_or_stopped(txq) ||
>> netpoll_start_xmit(skb, dev, txq) != NETDEV_TX_OK) {
>>
>>
>> Thanks for your patience.
>
> Eric,
>
> Let me know if you are okay with above changes and me sending v2.
Hi Tushar
Yes, this looks good to me, thanks !
^ permalink raw reply
* Re: [PATCH] brcm80211: brcmfmac: Ensure that incoming skb's are writable
From: Arend van Spriel @ 2017-04-20 19:13 UTC (permalink / raw)
To: James Hughes, Kalle Valo
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, Franky Lin, Hante Meuleman,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAE_XsMJGpKKdZ4E90tkzYwe+7KJ8JB5Rwrzga6aUi1LZG+wH4w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 4/20/2017 2:09 PM, James Hughes wrote:
> On 20 April 2017 at 12:31, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>> + linux-wireless
>>
>> James Hughes <james.hughes-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org> writes:
>>
>>> The driver was adding header information to incoming skb
>>> without ensuring the head was uncloned and hence writable.
>>>
>>> skb_cow_head has been used to ensure they are writable, however,
>>> this required some changes to error handling to ensure that
>>> if skb_cow_head failed it was not ignored.
>>>
>>> This really needs to be reviewed by someone who is more familiar
>>> with this code base to ensure any deallocation of skb's is
>>> still correct.
>>>
>>> Signed-off-by: James Hughes <james.hughes-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
>>
>> You should also CC linux-wireless, otherwise patchwork won't see it.
>>
>> --
>> Kalle Valo
>
> Thanks Kalle, I wasn't subscribed to wireless, but have now done so. I
> also failed to read the MAINTAINERS list correctly..
>
> With regard to this particular patch, this is related to the recent
> patches to use skb_cow_head in a number of USB net drivers to ensure
> they can write headers correctly, and I suspect the same fault is
> possible/likely in other drivers outside the USB net realm, as this
> patch shows.
>
> I'm not overly happy with the error handling in this patch, but that
> said, the error handling over this entire driver does strike me as
> suspect. Quite a few places where return codes are ignored, just in my
> quick examination. So not really sure how to proceed past this patch,
> if at all.
I would appreciate it if you can provide details about the code you
consider suspect. I will respond on the patches soon.
Regards,
Arend
^ permalink raw reply
* [PATCH V2 net] netdevice: Include NETIF_F_HW_CSUM when intersecting features
From: Vladislav Yasevich @ 2017-04-20 19:17 UTC (permalink / raw)
To: netdev; +Cc: Vladislav Yasevich, Michal Kubecek, Alexander Duyck, Tom Herbert
While hardware device use either NETIF_F_(IP|IPV6)_CSUM or
NETIF_F_HW_CSUM, all of the software devices use HW_CSUM.
This results in an interesting situation when the software
device is configured on top of hw device using (IP|IPV6)_CSUM.
In this situation, the user can't turn off checksum offloading
features on the software device.
This patch resolves that by adding NETIF_F_HW_CSUM to the mask
if a feature set includes only IP|IPV6 csum. This allows the user
to control the upper (software) device checksum, while at the same
time correctly propagating lower device changes up.
CC: Michal Kubecek <mkubecek@suse.cz>
CC: Alexander Duyck <alexander.duyck@gmail.com>
CC: Tom Herbert <tom@herbertland.com>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
V2: Addressed comments from Alex Duyck. I tested this with hacked virtio
device that set IP|IPV6 checksums instead of HW. Configuring a vlan on
top gave the vlan device with 'ip-generic: on' setting (using HW checksum).
This allows me to change vlan checksum offloads independent of virt-io nic.
Changes to virtio-nic propagated up to vlan, turning off the offloading
correctly.
include/linux/netdevice.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b0aa089..81aed2f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4009,10 +4009,10 @@ static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
netdev_features_t f2)
{
if ((f1 ^ f2) & NETIF_F_HW_CSUM) {
- if (f1 & NETIF_F_HW_CSUM)
- f1 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
- else
- f2 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
+ if (f1 & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM))
+ f1 |= NETIF_F_HW_CSUM;
+ if(f2 & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM))
+ f2 |= NETIF_F_HW_CSUM;
}
return f1 & f2;
--
2.7.4
^ permalink raw reply related
* [PATCH] Fix values type used in test_maps
From: David Miller @ 2017-04-20 19:20 UTC (permalink / raw)
To: ast; +Cc: borkmann, netdev
Maps of per-cpu type have their value element size adjusted to 8 if it
is specified smaller during various map operations.
This makes test_maps as a 32-bit binary fail, in fact the kernel
writes past the end of the value's array on the user's stack.
To be quite honest, I think the kernel should reject creation of a
per-cpu map that doesn't have a value size of at least 8 if that's
what the kernel is going to silently adjust to later.
If the user passed something smaller, it is a sizeof() calcualtion
based upon the type they will actually use (just like in this testcase
code) in later calls to the map operations.
Signed-off-by: David S. Miller <davem@davemloft.net>
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index a0aa200..20f1871 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -282,7 +282,7 @@ static void test_arraymap_percpu(int task, void *data)
{
unsigned int nr_cpus = bpf_num_possible_cpus();
int key, next_key, fd, i;
- long values[nr_cpus];
+ long long values[nr_cpus];
fd = bpf_create_map(BPF_MAP_TYPE_PERCPU_ARRAY, sizeof(key),
sizeof(values[0]), 2, 0);
@@ -340,7 +340,7 @@ static void test_arraymap_percpu_many_keys(void)
* allocator more than anything else
*/
unsigned int nr_keys = 2000;
- long values[nr_cpus];
+ long long values[nr_cpus];
int key, fd, i;
fd = bpf_create_map(BPF_MAP_TYPE_PERCPU_ARRAY, sizeof(key),
^ permalink raw reply related
* [PATCH netnext 1/3] selftests/net: cleanup unused parameter in psock_fanout
From: Mike Maloney @ 2017-04-20 19:25 UTC (permalink / raw)
To: netdev, davem; +Cc: Mike Maloney
In-Reply-To: <20170420192546.5567-1-maloneykernel@gmail.com>
From: Mike Maloney <maloney@google.com>
sock_fanout_open no longer sets the size of packet_socket ring, so stop
passing the parameter.
Tested:
Built and ran the test, it passed.
Signed-off-by: Mike Maloney <maloney@google.com>
Acked-by: Willem de Bruijn <willemb@google.com>
---
tools/testing/selftests/net/psock_fanout.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/net/psock_fanout.c b/tools/testing/selftests/net/psock_fanout.c
index 412459369686..b475d87d3aa3 100644
--- a/tools/testing/selftests/net/psock_fanout.c
+++ b/tools/testing/selftests/net/psock_fanout.c
@@ -71,7 +71,7 @@
/* Open a socket in a given fanout mode.
* @return -1 if mode is bad, a valid socket otherwise */
-static int sock_fanout_open(uint16_t typeflags, int num_packets)
+static int sock_fanout_open(uint16_t typeflags)
{
int fd, val;
@@ -210,7 +210,7 @@ static void test_control_single(void)
fprintf(stderr, "test: control single socket\n");
if (sock_fanout_open(PACKET_FANOUT_ROLLOVER |
- PACKET_FANOUT_FLAG_ROLLOVER, 0) != -1) {
+ PACKET_FANOUT_FLAG_ROLLOVER) != -1) {
fprintf(stderr, "ERROR: opened socket with dual rollover\n");
exit(1);
}
@@ -223,26 +223,26 @@ static void test_control_group(void)
fprintf(stderr, "test: control multiple sockets\n");
- fds[0] = sock_fanout_open(PACKET_FANOUT_HASH, 20);
+ fds[0] = sock_fanout_open(PACKET_FANOUT_HASH);
if (fds[0] == -1) {
fprintf(stderr, "ERROR: failed to open HASH socket\n");
exit(1);
}
if (sock_fanout_open(PACKET_FANOUT_HASH |
- PACKET_FANOUT_FLAG_DEFRAG, 10) != -1) {
+ PACKET_FANOUT_FLAG_DEFRAG) != -1) {
fprintf(stderr, "ERROR: joined group with wrong flag defrag\n");
exit(1);
}
if (sock_fanout_open(PACKET_FANOUT_HASH |
- PACKET_FANOUT_FLAG_ROLLOVER, 10) != -1) {
+ PACKET_FANOUT_FLAG_ROLLOVER) != -1) {
fprintf(stderr, "ERROR: joined group with wrong flag ro\n");
exit(1);
}
- if (sock_fanout_open(PACKET_FANOUT_CPU, 10) != -1) {
+ if (sock_fanout_open(PACKET_FANOUT_CPU) != -1) {
fprintf(stderr, "ERROR: joined group with wrong mode\n");
exit(1);
}
- fds[1] = sock_fanout_open(PACKET_FANOUT_HASH, 20);
+ fds[1] = sock_fanout_open(PACKET_FANOUT_HASH);
if (fds[1] == -1) {
fprintf(stderr, "ERROR: failed to join group\n");
exit(1);
@@ -263,8 +263,8 @@ static int test_datapath(uint16_t typeflags, int port_off,
fprintf(stderr, "test: datapath 0x%hx\n", typeflags);
- fds[0] = sock_fanout_open(typeflags, 20);
- fds[1] = sock_fanout_open(typeflags, 20);
+ fds[0] = sock_fanout_open(typeflags);
+ fds[1] = sock_fanout_open(typeflags);
if (fds[0] == -1 || fds[1] == -1) {
fprintf(stderr, "ERROR: failed open\n");
exit(1);
--
2.12.2.816.g2cccc81164-goog
^ permalink raw reply related
* [PATCH netnext 0/3] packet: Add option to create new fanout group with unique id.
From: Mike Maloney @ 2017-04-20 19:25 UTC (permalink / raw)
To: netdev, davem; +Cc: Mike Maloney
From: Mike Maloney <maloney@google.com>
Fanout uses a per net global namespace. A process that intends to create a
new fanout group can accidentally join an existing group. It is
not possible to detect this.
Add a socket option to specify on the first call to
setsockopt(..., PACKET_FANOUT, ...) to ensure that a new group is created.
Also add tests.
Mike Maloney (3):
selftests/net: cleanup unused parameter in psock_fanout
packet: add PACKET_FANOUT_FLAG_UNIQUEID to assign new fanout group id.
selftests/net: add tests for PACKET_FANOUT_FLAG_UNIQUEID
include/uapi/linux/if_packet.h | 1 +
net/packet/af_packet.c | 44 ++++++++++++++
tools/testing/selftests/net/psock_fanout.c | 93 ++++++++++++++++++++++++++----
3 files changed, 128 insertions(+), 10 deletions(-)
--
2.12.2.816.g2cccc81164-goog
^ 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