* [PATCH net-next RFC 2/2] tun: enable napi_gro_frags() for TUN/TAP driver
From: Petar Penkov @ 2017-09-05 22:35 UTC (permalink / raw)
To: netdev
Cc: Petar Penkov, Eric Dumazet, Mahesh Bandewar, Willem de Bruijn,
davem, ppenkov
In-Reply-To: <20170905223551.27925-1-ppenkov@google.com>
Add a TUN/TAP receive mode that exercises the napi_gro_frags()
interface. This mode is available only in TAP mode, as the interface
expects packets with Ethernet headers.
Furthermore, packets follow the layout of the iovec_iter that was
received. The first iovec is the linear data, and every one after the
first is a fragment. If there are more fragments than the max number,
drop the packet. Additionally, invoke eth_get_headlen() to exercise flow
dissector code and to verify that the header resides in the linear data.
The napi_gro_frags() mode requires setting the IFF_NAPI_FRAGS option.
This is imposed because this mode is intended for testing via tools like
syzkaller and packetdrill, and the increased flexibility it provides can
introduce security vulnerabilities.
Signed-off-by: Petar Penkov <ppenkov@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Mahesh Bandewar <maheshb@google.com>
Cc: Willem de Bruijn <willemb@google.com>
Cc: davem@davemloft.net
Cc: ppenkov@stanford.edu
---
drivers/net/tun.c | 135 ++++++++++++++++++++++++++++++++++++++++++--
include/uapi/linux/if_tun.h | 1 +
2 files changed, 130 insertions(+), 6 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index d5c824e3ec42..2ba9809ab6cd 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -75,6 +75,7 @@
#include <linux/skb_array.h>
#include <linux/bpf.h>
#include <linux/bpf_trace.h>
+#include <linux/mutex.h>
#include <linux/uaccess.h>
@@ -120,8 +121,15 @@ do { \
#define TUN_VNET_LE 0x80000000
#define TUN_VNET_BE 0x40000000
+#if IS_ENABLED(CONFIG_TUN_NAPI)
+#define TUN_FEATURES_EXTRA IFF_NAPI_FRAGS
+#else
+#define TUN_FEATURES_EXTRA 0
+#endif
+
#define TUN_FEATURES (IFF_NO_PI | IFF_ONE_QUEUE | IFF_VNET_HDR | \
- IFF_MULTI_QUEUE)
+ IFF_MULTI_QUEUE | TUN_FEATURES_EXTRA)
+
#define GOODCOPY_LEN 128
#define FLT_EXACT_COUNT 8
@@ -173,6 +181,7 @@ struct tun_file {
unsigned int ifindex;
};
struct napi_struct napi;
+ struct mutex napi_mutex; /* Protects access to the above napi */
struct list_head next;
struct tun_struct *detached;
struct skb_array tx_array;
@@ -276,6 +285,7 @@ static void tun_napi_init(struct tun_struct *tun, struct tun_file *tfile)
netif_napi_add(tun->dev, &tfile->napi, tun_napi_poll,
NAPI_POLL_WEIGHT);
napi_enable(&tfile->napi);
+ mutex_init(&tfile->napi_mutex);
}
}
@@ -291,6 +301,11 @@ static void tun_napi_del(struct tun_file *tfile)
netif_napi_del(&tfile->napi);
}
+static bool tun_napi_frags_enabled(const struct tun_struct *tun)
+{
+ return READ_ONCE(tun->flags) & IFF_NAPI_FRAGS;
+}
+
#ifdef CONFIG_TUN_VNET_CROSS_LE
static inline bool tun_legacy_is_little_endian(struct tun_struct *tun)
{
@@ -1034,7 +1049,8 @@ static void tun_poll_controller(struct net_device *dev)
* supports polling, which enables bridge devices in virt setups to
* still use netconsole
* If NAPI is enabled, however, we need to schedule polling for all
- * queues.
+ * queues unless we are using napi_gro_frags(), which we call in
+ * process context and not in NAPI context.
*/
if (IS_ENABLED(CONFIG_TUN_NAPI)) {
@@ -1042,6 +1058,9 @@ static void tun_poll_controller(struct net_device *dev)
struct tun_file *tfile;
int i;
+ if (tun_napi_frags_enabled(tun))
+ return;
+
rcu_read_lock();
for (i = 0; i < tun->numqueues; i++) {
tfile = rcu_dereference(tun->tfiles[i]);
@@ -1264,6 +1283,64 @@ static unsigned int tun_chr_poll(struct file *file, poll_table *wait)
return mask;
}
+static struct sk_buff *tun_napi_alloc_frags(struct tun_file *tfile,
+ size_t len,
+ const struct iov_iter *it)
+{
+ struct sk_buff *skb;
+ size_t linear;
+ int err;
+ int i;
+
+ if (it->nr_segs > MAX_SKB_FRAGS + 1)
+ return ERR_PTR(-ENOMEM);
+
+ local_bh_disable();
+ skb = napi_get_frags(&tfile->napi);
+ local_bh_enable();
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
+
+ linear = iov_iter_single_seg_count(it);
+ err = __skb_grow(skb, linear);
+ if (err)
+ goto free;
+
+ skb->len = len;
+ skb->data_len = len - linear;
+ skb->truesize += skb->data_len;
+
+ for (i = 1; i < it->nr_segs; i++) {
+ size_t fragsz = it->iov[i].iov_len;
+ unsigned long offset;
+ struct page *page;
+ void *data;
+
+ if (fragsz == 0 || fragsz > PAGE_SIZE) {
+ err = -EINVAL;
+ goto free;
+ }
+
+ local_bh_disable();
+ data = napi_alloc_frag(fragsz);
+ local_bh_enable();
+ if (!data) {
+ err = -ENOMEM;
+ goto free;
+ }
+
+ page = virt_to_page(data);
+ offset = offset_in_page(data);
+ skb_fill_page_desc(skb, i - 1, page, offset, fragsz);
+ }
+
+ return skb;
+free:
+ /* frees skb and all frags allocated with napi_alloc_frag() */
+ napi_free_frags(&tfile->napi);
+ return ERR_PTR(err);
+}
+
/* prepad is the amount to reserve at front. len is length after that.
* linear is a hint as to how much to copy (usually headers). */
static struct sk_buff *tun_alloc_skb(struct tun_file *tfile,
@@ -1466,6 +1543,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
int err;
u32 rxhash;
int generic_xdp = 1;
+ bool frags = tun_napi_frags_enabled(tun);
if (!(tun->dev->flags & IFF_UP))
return -EIO;
@@ -1523,7 +1601,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
zerocopy = true;
}
- if (tun_can_build_skb(tun, tfile, len, noblock, zerocopy)) {
+ if (!frags && tun_can_build_skb(tun, tfile, len, noblock, zerocopy)) {
skb = tun_build_skb(tun, tfile, from, &gso, len, &generic_xdp);
if (IS_ERR(skb)) {
this_cpu_inc(tun->pcpu_stats->rx_dropped);
@@ -1540,10 +1618,24 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
linear = tun16_to_cpu(tun, gso.hdr_len);
}
- skb = tun_alloc_skb(tfile, align, copylen, linear, noblock);
+ if (frags) {
+ mutex_lock(&tfile->napi_mutex);
+ skb = tun_napi_alloc_frags(tfile, copylen, from);
+ /* tun_napi_alloc_frags() enforces a layout for the skb.
+ * If zerocopy is enabled, then this layout will be
+ * overwritten by zerocopy_sg_from_iter().
+ */
+ zerocopy = false;
+ } else {
+ skb = tun_alloc_skb(tfile, align, copylen, linear,
+ noblock);
+ }
+
if (IS_ERR(skb)) {
if (PTR_ERR(skb) != -EAGAIN)
this_cpu_inc(tun->pcpu_stats->rx_dropped);
+ if (frags)
+ mutex_unlock(&tfile->napi_mutex);
return PTR_ERR(skb);
}
@@ -1555,6 +1647,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
if (err) {
this_cpu_inc(tun->pcpu_stats->rx_dropped);
kfree_skb(skb);
+ if (frags) {
+ tfile->napi.skb = NULL;
+ mutex_unlock(&tfile->napi_mutex);
+ }
+
return -EFAULT;
}
}
@@ -1562,6 +1659,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun))) {
this_cpu_inc(tun->pcpu_stats->rx_frame_errors);
kfree_skb(skb);
+ if (frags) {
+ tfile->napi.skb = NULL;
+ mutex_unlock(&tfile->napi_mutex);
+ }
+
return -EINVAL;
}
@@ -1587,7 +1689,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
skb->dev = tun->dev;
break;
case IFF_TAP:
- skb->protocol = eth_type_trans(skb, tun->dev);
+ if (!frags)
+ skb->protocol = eth_type_trans(skb, tun->dev);
break;
}
@@ -1622,7 +1725,23 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
rxhash = __skb_get_hash_symmetric(skb);
- if (IS_ENABLED(CONFIG_TUN_NAPI)) {
+ if (frags) {
+ /* Exercise flow dissector code path. */
+ u32 headlen = eth_get_headlen(skb->data, skb_headlen(skb));
+
+ if (headlen > skb_headlen(skb) || headlen < ETH_HLEN) {
+ this_cpu_inc(tun->pcpu_stats->rx_dropped);
+ napi_free_frags(&tfile->napi);
+ mutex_unlock(&tfile->napi_mutex);
+ WARN_ON(1);
+ return -ENOMEM;
+ }
+
+ local_bh_disable();
+ napi_gro_frags(&tfile->napi);
+ local_bh_enable();
+ mutex_unlock(&tfile->napi_mutex);
+ } else if (IS_ENABLED(CONFIG_TUN_NAPI)) {
struct sk_buff_head *queue = &tfile->sk.sk_write_queue;
int queue_len;
@@ -2168,6 +2287,10 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
tun->flags = (tun->flags & ~TUN_FEATURES) |
(ifr->ifr_flags & TUN_FEATURES);
+ if (!IS_ENABLED(CONFIG_TUN_NAPI) ||
+ (tun->flags & TUN_TYPE_MASK) != IFF_TAP)
+ tun->flags = tun->flags & ~IFF_NAPI_FRAGS;
+
/* Make sure persistent devices do not get stuck in
* xoff state.
*/
diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h
index 3cb5e1d85ddd..1eb1eb42f151 100644
--- a/include/uapi/linux/if_tun.h
+++ b/include/uapi/linux/if_tun.h
@@ -60,6 +60,7 @@
/* TUNSETIFF ifr flags */
#define IFF_TUN 0x0001
#define IFF_TAP 0x0002
+#define IFF_NAPI_FRAGS 0x0010
#define IFF_NO_PI 0x1000
/* This flag has no real effect */
#define IFF_ONE_QUEUE 0x2000
--
2.14.1.581.gf28d330327-goog
^ permalink raw reply related
* Re: [RFC net-next] net: sch_clsact: add support for global per-netns classifier mode
From: Daniel Borkmann @ 2017-09-05 22:45 UTC (permalink / raw)
To: Roopa Prabhu, Cong Wang
Cc: Nikolay Aleksandrov, Linux Kernel Network Developers, David Ahern,
Jiri Pirko, Jamal Hadi Salim
In-Reply-To: <CAJieiUj_-o-Cg2AzN5b0vrdnBD69ryMmpu4fcC64-MHEmQ-u4g@mail.gmail.com>
On 09/06/2017 12:01 AM, Roopa Prabhu wrote:
> On Tue, Sep 5, 2017 at 11:18 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Tue, Sep 5, 2017 at 5:48 AM, Nikolay Aleksandrov
>> <nikolay@cumulusnetworks.com> wrote:
>>> Hi all,
>>> This RFC adds a new mode for clsact which designates a device's egress
>>> classifier as global per netns. The packets that are not classified for
>>> a particular device will be classified using the global classifier.
>>> We have needed a global classifier for some time now for various
>>> purposes and setting the single bridge or loopback/vrf device as the
Can you elaborate a bit more on the ... "we have needed a global
classifier for some time now for various purposes".
>>> global classifier device is acceptable for us. Doing it this way avoids
>>> the act/cls device and queue dependencies.
>>>
>>> This is strictly an RFC patch just to show the intent, if we agree on
>>> the details the proposed patch will have support for both ingress and
>>> egress, and will be using a static key to avoid the fast path test when no
>>> global classifier has been configured.
>>>
>>> Example (need a modified tc that adds TCA_OPTIONS when using q_clsact):
>>> $ tc qdisc add dev lo clsact global
>>> $ tc filter add dev lo egress protocol ip u32 match ip dst 4.3.2.1/32 action drop
>>>
>>> the last filter will be global for all devices that don't have a
>>> specific egress_cl_list (i.e. have clsact configured).
>>
>> Sorry this is too ugly.
+1
>> netdevice is still implied in your command line even if you treat it
>> as global. It is essentially hard to bypass netdevice layer since
>> netdevice is the core of L2 and also where everything begins.
>>
>> Maybe the best we can do here is make tc filters standalone
>> as tc actions so that filters can exist before qdisc's and netdevices.
>> But this probably requires significant works to make it working
>> with both existing non-standalone and bindings standalones
>> with qdisc's.
>
> yes, like Nikolay says we have been discussing this as well. Nikolay's
> patch is a cleaver and most importantly non-invasive
> way today given the anchor point for tc rules is a netdev. we have
> also considered a separate implicit tc anchor device.
Seems ugly just as well. :( Hmm, why not just having the two list
pointers (ingress, egress list) in the netns struct and when
something configures them to be effectively non-zero, then devices
in that netns could automatically get a clsact and inherit the
lists from there such that sch_handle_ingress() and sch_handle_egress()
require exactly zero changes in fast-path. You could then go and
say that either you would make changes to clsact for individual
devices immutable when they use the 'shared' list pointers, or then
duplicate the configs when being altered from the global one. Would
push the complexity to control path only at least. Just a brief
thought.
^ permalink raw reply
* Re: [PATCH net-next RFC 1/2] tun: enable NAPI for TUN/TAP driver
From: Stephen Hemminger @ 2017-09-05 22:51 UTC (permalink / raw)
To: Petar Penkov
Cc: netdev, Eric Dumazet, Mahesh Bandewar, Willem de Bruijn, davem,
ppenkov
In-Reply-To: <20170905223551.27925-2-ppenkov@google.com>
On Tue, 5 Sep 2017 15:35:50 -0700
Petar Penkov <ppenkov@google.com> wrote:
> Changes TUN driver to use napi_gro_receive() upon receiving packets
> rather than netif_rx_ni(). Adds flag CONFIG_TUN_NAPI that enables
> these changes and operation is not affected if the flag is disabled.
> SKBs are constructed upon packet arrival and are queued to be
> processed later.
>
> The new path was evaluated with a benchmark with the following setup:
> Open two tap devices and a receiver thread that reads in a loop for
> each device. Start one sender thread and pin all threads to different
> CPUs. Send 1M minimum UDP packets to each device and measure sending
> time for each of the sending methods:
> napi_gro_receive(): 4.90s
> netif_rx_ni(): 4.90s
> netif_receive_skb(): 7.20s
>
> Signed-off-by: Petar Penkov <ppenkov@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Mahesh Bandewar <maheshb@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Cc: davem@davemloft.net
> Cc: ppenkov@stanford.edu
Why is this optional? It adds two code paths both of which need
to be tested.
^ permalink raw reply
* Re: linux-next: manual merge of the pci tree with the net tree
From: Sinan Kaya @ 2017-09-05 22:54 UTC (permalink / raw)
To: Stephen Rothwell, Bjorn Helgaas, David Miller, Networking
Cc: Linux-Next Mailing List, Linux Kernel Mailing List, Ding Tianhong,
Casey Leedom
In-Reply-To: <20170904145427.60b35b0c@canb.auug.org.au>
On 9/4/2017 12:54 AM, Stephen Rothwell wrote:
> Just a reminder that this conflict still exists.
I suppose this was a message for Bjorn, right?
I looked at the change and it looked good to me.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: linux-next: manual merge of the pci tree with the net tree
From: Stephen Rothwell @ 2017-09-05 23:10 UTC (permalink / raw)
To: Sinan Kaya
Cc: Bjorn Helgaas, David Miller, Networking, Linux-Next Mailing List,
Linux Kernel Mailing List, Ding Tianhong, Casey Leedom
In-Reply-To: <22fb2201-6ecd-bfc9-9f65-4807d7bf36a1@codeaurora.org>
Hi Sinan,
On Tue, 5 Sep 2017 18:54:38 -0400 Sinan Kaya <okaya@codeaurora.org> wrote:
>
> On 9/4/2017 12:54 AM, Stephen Rothwell wrote:
> > Just a reminder that this conflict still exists.
>
> I suppose this was a message for Bjorn, right?
Correct. It was just a reminder that he may have to tell Linus about
the conflict when he asks Linus to merge his tree (depending on how
complex the conflict is and if he gets in before Dave gets the net-next
tree merged).
--
Cheers,
Stephen Rothwell
^ permalink raw reply
* Re: [RFC net-next] net: sch_clsact: add support for global per-netns classifier mode
From: Daniel Borkmann @ 2017-09-05 23:12 UTC (permalink / raw)
To: Roopa Prabhu, Cong Wang
Cc: Nikolay Aleksandrov, Linux Kernel Network Developers, David Ahern,
Jiri Pirko, Jamal Hadi Salim
In-Reply-To: <59AF291E.90508@iogearbox.net>
On 09/06/2017 12:45 AM, Daniel Borkmann wrote:
> On 09/06/2017 12:01 AM, Roopa Prabhu wrote:
>> On Tue, Sep 5, 2017 at 11:18 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Tue, Sep 5, 2017 at 5:48 AM, Nikolay Aleksandrov
>>> <nikolay@cumulusnetworks.com> wrote:
>>>> Hi all,
>>>> This RFC adds a new mode for clsact which designates a device's egress
>>>> classifier as global per netns. The packets that are not classified for
>>>> a particular device will be classified using the global classifier.
>>>> We have needed a global classifier for some time now for various
>>>> purposes and setting the single bridge or loopback/vrf device as the
>
> Can you elaborate a bit more on the ... "we have needed a global
> classifier for some time now for various purposes".
>
>>>> global classifier device is acceptable for us. Doing it this way avoids
>>>> the act/cls device and queue dependencies.
>>>>
>>>> This is strictly an RFC patch just to show the intent, if we agree on
>>>> the details the proposed patch will have support for both ingress and
>>>> egress, and will be using a static key to avoid the fast path test when no
>>>> global classifier has been configured.
>>>>
>>>> Example (need a modified tc that adds TCA_OPTIONS when using q_clsact):
>>>> $ tc qdisc add dev lo clsact global
>>>> $ tc filter add dev lo egress protocol ip u32 match ip dst 4.3.2.1/32 action drop
>>>>
>>>> the last filter will be global for all devices that don't have a
>>>> specific egress_cl_list (i.e. have clsact configured).
>>>
>>> Sorry this is too ugly.
>
> +1
>
>>> netdevice is still implied in your command line even if you treat it
>>> as global. It is essentially hard to bypass netdevice layer since
>>> netdevice is the core of L2 and also where everything begins.
One could probably use special wildcard device name, e.g. tcpdump
allows for 'tcpdump -i any' to capture on all devices, so you could
indicate something like 'tc filter add dev any blah' to have similar
semantics in the realm of the netns w/o making it look too hacky
for tc users perhaps.
>>> Maybe the best we can do here is make tc filters standalone
>>> as tc actions so that filters can exist before qdisc's and netdevices.
>>> But this probably requires significant works to make it working
>>> with both existing non-standalone and bindings standalones
>>> with qdisc's.
>>
>> yes, like Nikolay says we have been discussing this as well. Nikolay's
>> patch is a cleaver and most importantly non-invasive
>> way today given the anchor point for tc rules is a netdev. we have
>> also considered a separate implicit tc anchor device.
>
> Seems ugly just as well. :( Hmm, why not just having the two list
> pointers (ingress, egress list) in the netns struct and when
> something configures them to be effectively non-zero, then devices
> in that netns could automatically get a clsact and inherit the
> lists from there such that sch_handle_ingress() and sch_handle_egress()
> require exactly zero changes in fast-path. You could then go and
> say that either you would make changes to clsact for individual
> devices immutable when they use the 'shared' list pointers, or then
> duplicate the configs when being altered from the global one. Would
> push the complexity to control path only at least. Just a brief
> thought.
^ permalink raw reply
* [PATCH v2 rfc 2/8] net: bridge: Send notification when host join/leaves a group
From: Andrew Lunn @ 2017-09-05 23:35 UTC (permalink / raw)
To: netdev; +Cc: jiri, nikolay, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
The host can join or leave a multicast group on the brX interface, as
indicated by IGMP snooping. This is tracked within the bridge
multicast code. Send a notification when this happens, in the same way
a notification is sent when a port of the bridge joins/leaves a group
because of IGMP snooping.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
net/bridge/br_mdb.c | 9 ++++++---
net/bridge/br_multicast.c | 6 +++++-
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 71885e251988..80fa91ccc50c 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -316,7 +316,7 @@ static void __br_mdb_notify(struct net_device *dev, struct net_bridge_port *p,
#endif
mdb.obj.orig_dev = port_dev;
- if (port_dev && type == RTM_NEWMDB) {
+ if (p && port_dev && type == RTM_NEWMDB) {
complete_info = kmalloc(sizeof(*complete_info), GFP_ATOMIC);
if (complete_info) {
complete_info->port = p;
@@ -326,7 +326,7 @@ static void __br_mdb_notify(struct net_device *dev, struct net_bridge_port *p,
if (switchdev_port_obj_add(port_dev, &mdb.obj))
kfree(complete_info);
}
- } else if (port_dev && type == RTM_DELMDB) {
+ } else if (p && port_dev && type == RTM_DELMDB) {
switchdev_port_obj_del(port_dev, &mdb.obj);
}
@@ -352,7 +352,10 @@ void br_mdb_notify(struct net_device *dev, struct net_bridge_port *port,
struct br_mdb_entry entry;
memset(&entry, 0, sizeof(entry));
- entry.ifindex = port->dev->ifindex;
+ if (port)
+ entry.ifindex = port->dev->ifindex;
+ else
+ entry.ifindex = dev->ifindex;
entry.addr.proto = group->proto;
entry.addr.u.ip4 = group->u.ip4;
#if IS_ENABLED(CONFIG_IPV6)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index c6b2b8d419e7..955f340fe719 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -250,6 +250,7 @@ static void br_multicast_group_expired(unsigned long data)
goto out;
mp->host_joined = false;
+ br_mdb_notify(br->dev, NULL, &mp->addr, RTM_DELMDB, 0);
if (mp->ports)
goto out;
@@ -775,7 +776,10 @@ static int br_multicast_add_group(struct net_bridge *br,
goto err;
if (!port) {
- mp->host_joined = true;
+ if (!mp->host_joined) {
+ mp->host_joined = true;
+ br_mdb_notify(br->dev, NULL, &mp->addr, RTM_NEWMDB, 0);
+ }
mod_timer(&mp->timer, now + br->multicast_membership_interval);
goto out;
}
--
2.14.1
^ permalink raw reply related
* [PATCH v2 rfc 7/8] net: dsa: set offload_fwd_mark on received packets
From: Andrew Lunn @ 2017-09-05 23:35 UTC (permalink / raw)
To: netdev; +Cc: jiri, nikolay, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
The software bridge needs to know if a packet has already been bridged
by hardware offload to ports in the same hardware offload, in order
that it does not re-flood them, causing duplicates. This is
particularly true for multicast traffic which the host has required.
By setting offload_fwd_mark in the skb the bridge will only flood to
ports in other offloads and other netifs.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
net/dsa/dsa.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 03c58b0eb082..5732696ac71c 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -213,6 +213,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
skb_push(skb, ETH_HLEN);
skb->pkt_type = PACKET_HOST;
skb->protocol = eth_type_trans(skb, skb->dev);
+ skb->offload_fwd_mark = 1;
s = this_cpu_ptr(p->stats64);
u64_stats_update_begin(&s->syncp);
--
2.14.1
^ permalink raw reply related
* [PATCH v2 rfc 5/8] net: dsa: switch: handle host mdb add/remove
From: Andrew Lunn @ 2017-09-05 23:35 UTC (permalink / raw)
To: netdev; +Cc: jiri, nikolay, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
When receiving notifications of the host mdb add/remove, add/remove an
mdb to the CPU port, so that traffic flows from a port to the CPU port
and hence to the host.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
net/dsa/switch.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 60 insertions(+), 12 deletions(-)
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index e6c06aa349a6..326680039c52 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -108,22 +108,14 @@ static int dsa_switch_fdb_del(struct dsa_switch *ds,
info->vid);
}
-static int dsa_switch_mdb_add(struct dsa_switch *ds,
- struct dsa_notifier_mdb_info *info)
+static int dsa_switch_mdb_add_bitmap(struct dsa_switch *ds,
+ struct dsa_notifier_mdb_info *info,
+ const struct switchdev_obj_port_mdb *mdb,
+ unsigned long *group)
{
- const struct switchdev_obj_port_mdb *mdb = info->mdb;
struct switchdev_trans *trans = info->trans;
- DECLARE_BITMAP(group, ds->num_ports);
int port, err;
- /* Build a mask of Multicast group members */
- bitmap_zero(group, ds->num_ports);
- if (ds->index == info->sw_index)
- set_bit(info->port, group);
- for (port = 0; port < ds->num_ports; port++)
- if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
- set_bit(port, group);
-
if (switchdev_trans_ph_prepare(trans)) {
if (!ds->ops->port_mdb_prepare || !ds->ops->port_mdb_add)
return -EOPNOTSUPP;
@@ -141,6 +133,40 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
return 0;
}
+static int dsa_switch_mdb_add(struct dsa_switch *ds,
+ struct dsa_notifier_mdb_info *info)
+{
+ const struct switchdev_obj_port_mdb *mdb = info->mdb;
+ DECLARE_BITMAP(group, ds->num_ports);
+ int port;
+
+ /* Build a mask of Multicast group members */
+ bitmap_zero(group, ds->num_ports);
+ if (ds->index == info->sw_index)
+ set_bit(info->port, group);
+ for (port = 0; port < ds->num_ports; port++)
+ if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+ set_bit(port, group);
+
+ return dsa_switch_mdb_add_bitmap(ds, info, mdb, group);
+}
+
+static int dsa_switch_host_mdb_add(struct dsa_switch *ds,
+ struct dsa_notifier_mdb_info *info)
+{
+ const struct switchdev_obj_port_mdb *mdb = info->mdb;
+ DECLARE_BITMAP(group, ds->num_ports);
+ int port;
+
+ /* Build a mask of Multicast group members */
+ bitmap_zero(group, ds->num_ports);
+ for (port = 0; port < ds->num_ports; port++)
+ if (dsa_is_cpu_port(ds, port))
+ set_bit(port, group);
+
+ return dsa_switch_mdb_add_bitmap(ds, info, mdb, group);
+}
+
static int dsa_switch_mdb_del(struct dsa_switch *ds,
struct dsa_notifier_mdb_info *info)
{
@@ -155,6 +181,22 @@ static int dsa_switch_mdb_del(struct dsa_switch *ds,
return 0;
}
+static int dsa_switch_host_mdb_del(struct dsa_switch *ds,
+ struct dsa_notifier_mdb_info *info)
+{
+ const struct switchdev_obj_port_mdb *mdb = info->mdb;
+ int port;
+
+ if (!ds->ops->port_mdb_del)
+ return -EOPNOTSUPP;
+
+ for (port = 0; port < ds->num_ports; port++)
+ if (dsa_is_cpu_port(ds, port))
+ ds->ops->port_mdb_del(ds, port, mdb);
+
+ return 0;
+}
+
static int dsa_switch_vlan_add(struct dsa_switch *ds,
struct dsa_notifier_vlan_info *info)
{
@@ -230,6 +272,12 @@ static int dsa_switch_event(struct notifier_block *nb,
case DSA_NOTIFIER_MDB_DEL:
err = dsa_switch_mdb_del(ds, info);
break;
+ case DSA_NOTIFIER_HOST_MDB_ADD:
+ err = dsa_switch_host_mdb_add(ds, info);
+ break;
+ case DSA_NOTIFIER_HOST_MDB_DEL:
+ err = dsa_switch_host_mdb_del(ds, info);
+ break;
case DSA_NOTIFIER_VLAN_ADD:
err = dsa_switch_vlan_add(ds, info);
break;
--
2.14.1
^ permalink raw reply related
* [PATCH v2 rfc 8/8] net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID
From: Andrew Lunn @ 2017-09-05 23:35 UTC (permalink / raw)
To: netdev; +Cc: jiri, nikolay, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
SWITCHDEV_ATTR_ID_PORT_PARENT_ID is used by the software bridge when
determining which ports to flood a packet out. If the packet
originated from a switch, it assumes the switch has already flooded
the packet out the switches ports, so the bridge should not flood the
packet itself out switch ports. Ports on the same switch are expected
to return the same parent ID when SWITCHDEV_ATTR_ID_PORT_PARENT_ID is
called.
DSA gets this wrong with clusters of switches. As far as the software
bridge is concerned, the cluster is all one switch. A packet from any
switch in the cluster can be assumed to of been flooded as needed out
all ports of the cluster, not just the switch it originated
from. Hence all ports of a cluster should return the same parent. The
old implementation did not, each switch in the cluster had its own ID.
Also wrong was that the ID was not unique if multiple DSA instances
are in operation.
Use the MAC address of the master interface as the parent ID. This is
the same for all switches in a cluster, and should be unique if there
are multiple clusters.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
net/dsa/slave.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2e07be149415..d2744b0dad6e 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -374,13 +374,15 @@ static int dsa_slave_port_attr_get(struct net_device *dev,
struct switchdev_attr *attr)
{
struct dsa_slave_priv *p = netdev_priv(dev);
- struct dsa_switch *ds = p->dp->ds;
switch (attr->id) {
- case SWITCHDEV_ATTR_ID_PORT_PARENT_ID:
- attr->u.ppid.id_len = sizeof(ds->index);
- memcpy(&attr->u.ppid.id, &ds->index, attr->u.ppid.id_len);
+ case SWITCHDEV_ATTR_ID_PORT_PARENT_ID: {
+ struct net_device *master = dsa_master_netdev(p);
+
+ attr->u.ppid.id_len = ETH_ALEN;
+ ether_addr_copy(attr->u.ppid.id, master->dev_addr);
break;
+ }
case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT:
attr->u.brport_flags_support = 0;
break;
--
2.14.1
^ permalink raw reply related
* [PATCH v2 rfc 4/8] net: dsa: slave: Handle switchdev host mdb add/del
From: Andrew Lunn @ 2017-09-05 23:35 UTC (permalink / raw)
To: netdev; +Cc: jiri, nikolay, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
Add code to handle switchdev host mdb add/del. As with normal mdb
add/del, send a notification to the switch layer.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
net/dsa/dsa_priv.h | 7 +++++++
net/dsa/port.c | 26 ++++++++++++++++++++++++++
net/dsa/slave.c | 6 ++++++
3 files changed, 39 insertions(+)
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 9c3eeb72462d..0ffe49f78d14 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -24,6 +24,8 @@ enum {
DSA_NOTIFIER_FDB_DEL,
DSA_NOTIFIER_MDB_ADD,
DSA_NOTIFIER_MDB_DEL,
+ DSA_NOTIFIER_HOST_MDB_ADD,
+ DSA_NOTIFIER_HOST_MDB_DEL,
DSA_NOTIFIER_VLAN_ADD,
DSA_NOTIFIER_VLAN_DEL,
};
@@ -131,6 +133,11 @@ int dsa_port_mdb_add(struct dsa_port *dp,
struct switchdev_trans *trans);
int dsa_port_mdb_del(struct dsa_port *dp,
const struct switchdev_obj_port_mdb *mdb);
+int dsa_host_mdb_add(struct dsa_port *dp,
+ const struct switchdev_obj_port_mdb *mdb,
+ struct switchdev_trans *trans);
+int dsa_host_mdb_del(struct dsa_port *dp,
+ const struct switchdev_obj_port_mdb *mdb);
int dsa_port_vlan_add(struct dsa_port *dp,
const struct switchdev_obj_port_vlan *vlan,
struct switchdev_trans *trans);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 659676ba3f8b..5b18b9fe2219 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -199,6 +199,32 @@ int dsa_port_mdb_del(struct dsa_port *dp,
return dsa_port_notify(dp, DSA_NOTIFIER_MDB_DEL, &info);
}
+int dsa_host_mdb_add(struct dsa_port *dp,
+ const struct switchdev_obj_port_mdb *mdb,
+ struct switchdev_trans *trans)
+{
+ struct dsa_notifier_mdb_info info = {
+ .sw_index = dp->ds->index,
+ .port = dp->index,
+ .trans = trans,
+ .mdb = mdb,
+ };
+
+ return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_ADD, &info);
+}
+
+int dsa_host_mdb_del(struct dsa_port *dp,
+ const struct switchdev_obj_port_mdb *mdb)
+{
+ struct dsa_notifier_mdb_info info = {
+ .sw_index = dp->ds->index,
+ .port = dp->index,
+ .mdb = mdb,
+ };
+
+ return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_DEL, &info);
+}
+
int dsa_port_vlan_add(struct dsa_port *dp,
const struct switchdev_obj_port_vlan *vlan,
struct switchdev_trans *trans)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 78e78a6e6833..2e07be149415 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -330,6 +330,9 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
case SWITCHDEV_OBJ_ID_PORT_MDB:
err = dsa_port_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj), trans);
break;
+ case SWITCHDEV_OBJ_ID_HOST_MDB:
+ err = dsa_host_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj), trans);
+ break;
case SWITCHDEV_OBJ_ID_PORT_VLAN:
err = dsa_port_vlan_add(dp, SWITCHDEV_OBJ_PORT_VLAN(obj),
trans);
@@ -353,6 +356,9 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
case SWITCHDEV_OBJ_ID_PORT_MDB:
err = dsa_port_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
break;
+ case SWITCHDEV_OBJ_ID_HOST_MDB:
+ err = dsa_host_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
+ break;
case SWITCHDEV_OBJ_ID_PORT_VLAN:
err = dsa_port_vlan_del(dp, SWITCHDEV_OBJ_PORT_VLAN(obj));
break;
--
2.14.1
^ permalink raw reply related
* [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Andrew Lunn @ 2017-09-05 23:35 UTC (permalink / raw)
To: netdev; +Cc: jiri, nikolay, Florian Fainelli, Vivien Didelot, Andrew Lunn
After the very useful feedback from Nikolay, i threw away what i had,
and started again. To recap:
The linux bridge supports IGMP snooping. It will listen to IGMP
reports on bridge ports and keep track of which groups have been
joined on an interface. It will then forward multicast based on this
group membership.
When the bridge adds or removed groups from an interface, it uses
switchdev to request the hardware add an mdb to a port, so the
hardware can perform the selective forwarding between ports.
What is not covered by the current bridge code, is IGMP joins/leaves
from the host on the brX interface. These are not reported via
switchdev so that hardware knows the local host is interested in the
multicast frames.
Luckily, the bridge does track joins/leaves on the brX interface. The
code is obfusticated, which is why i missed it with my first attempt.
So the first patch tries to remove this obfustication. Currently,
there is no notifications sent when the bridge interface joins a
group. The second patch adds them. bridge monitor then shows
joins/leaves in the same way as for other ports of the bridge.
Then starts the work passing down to the hardware that the host has
joined/left a group. The existing switchdev mdb object cannot be used,
since the semantics are different. The existing
SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
group should be forwarded out that port of the switch. However here we
require the exact opposite. We want multicast frames for the group
received on the port to the forwarded to the host. Hence add a new
object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
forward to the host. This new object is then propagated through the
DSA layers. No DSA driver changes should be needed, this should just
work...
Getting the frames to the bridge as requested turned up an issue or
three. The offload_fwd_mark is not being set by DSA, so the bridge
floods the received frames back to the switch ports, resulting in
duplication since the hardware has already flooded the packet. Fixing
that turned up an issue with the meaning of
SWITCHDEV_ATTR_ID_PORT_PARENT_ID in DSA. A DSA fabric of three
switches needs to look to the software bridge as a single
switch. Otherwise the offload_fwd_mark does not work, and we get
duplication on the non-ingress switch. But each switch returned a
different value. And they were not unique.
The third and last issue will be explained in a followup email.
Open questions:
Is sending notifications going to break userspace?
Is this new switchdev object O.K. for the few non-DSA switches that exist?
Is the SWITCHDEV_ATTR_ID_PORT_PARENT_ID change acceptable?
Andrew
Andrew Lunn (8):
net: bridge: Rename mglist to host_joined
net: bridge: Send notification when host join/leaves a group
net: bridge: Add/del switchdev object on host join/leave
net: dsa: slave: Handle switchdev host mdb add/del
net: dsa: switch: handle host mdb add/remove
net: dsa: switch: Don't add CPU port to an mdb by default
net: dsa: set offload_fwd_mark on received packets
net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID
include/net/switchdev.h | 1 +
net/bridge/br_input.c | 2 +-
net/bridge/br_mdb.c | 50 +++++++++++++++++++++++++++++---
net/bridge/br_multicast.c | 18 +++++++-----
net/bridge/br_private.h | 2 +-
net/dsa/dsa.c | 1 +
net/dsa/dsa_priv.h | 7 +++++
net/dsa/port.c | 26 +++++++++++++++++
net/dsa/slave.c | 16 ++++++++---
net/dsa/switch.c | 72 +++++++++++++++++++++++++++++++++++++++--------
net/switchdev/switchdev.c | 2 ++
11 files changed, 168 insertions(+), 29 deletions(-)
--
2.14.1
^ permalink raw reply
* [PATCH v2 rfc 6/8] net: dsa: switch: Don't add CPU port to an mdb by default
From: Andrew Lunn @ 2017-09-05 23:35 UTC (permalink / raw)
To: netdev; +Cc: jiri, nikolay, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
Now that the host indicates when a multicast group should be forwarded
from the switch to the host, don't do it by default.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
net/dsa/switch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 326680039c52..147295d8de94 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -145,7 +145,7 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
if (ds->index == info->sw_index)
set_bit(info->port, group);
for (port = 0; port < ds->num_ports; port++)
- if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+ if (dsa_is_dsa_port(ds, port))
set_bit(port, group);
return dsa_switch_mdb_add_bitmap(ds, info, mdb, group);
--
2.14.1
^ permalink raw reply related
* [PATCH v2 rfc 3/8] net: bridge: Add/del switchdev object on host join/leave
From: Andrew Lunn @ 2017-09-05 23:35 UTC (permalink / raw)
To: netdev; +Cc: jiri, nikolay, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
When the host joins or leaves a multicast group, use switchdev to add
an object to the hardware to forward traffic for the group to the
host.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
include/net/switchdev.h | 1 +
net/bridge/br_mdb.c | 39 +++++++++++++++++++++++++++++++++++++++
net/switchdev/switchdev.c | 2 ++
3 files changed, 42 insertions(+)
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index d767b7991887..b298a6b4c11d 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -75,6 +75,7 @@ enum switchdev_obj_id {
SWITCHDEV_OBJ_ID_UNDEFINED,
SWITCHDEV_OBJ_ID_PORT_VLAN,
SWITCHDEV_OBJ_ID_PORT_MDB,
+ SWITCHDEV_OBJ_ID_HOST_MDB,
};
struct switchdev_obj {
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 80fa91ccc50c..a9c03f3a3b88 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -291,6 +291,42 @@ static void br_mdb_complete(struct net_device *dev, int err, void *priv)
kfree(priv);
}
+static void br_mdb_switchdev_host_port(struct net_device *dev,
+ struct net_device *lower_dev,
+ struct br_mdb_entry *entry, int type)
+{
+ struct switchdev_obj_port_mdb mdb = {
+ .obj = {
+ .id = SWITCHDEV_OBJ_ID_HOST_MDB,
+ .flags = SWITCHDEV_F_DEFER,
+ },
+ .vid = entry->vid,
+ };
+
+ if (entry->addr.proto == htons(ETH_P_IP))
+ ip_eth_mc_map(entry->addr.u.ip4, mdb.addr);
+#if IS_ENABLED(CONFIG_IPV6)
+ else
+ ipv6_eth_mc_map(&entry->addr.u.ip6, mdb.addr);
+#endif
+
+ mdb.obj.orig_dev = dev;
+ if (type == RTM_NEWMDB)
+ switchdev_port_obj_add(lower_dev, &mdb.obj);
+ if (type == RTM_DELMDB)
+ switchdev_port_obj_del(lower_dev, &mdb.obj);
+}
+
+static void br_mdb_switchdev_host(struct net_device *dev,
+ struct br_mdb_entry *entry, int type)
+{
+ struct net_device *lower_dev;
+ struct list_head *iter;
+
+ netdev_for_each_lower_dev(dev, lower_dev, iter)
+ br_mdb_switchdev_host_port(dev, lower_dev, entry, type);
+}
+
static void __br_mdb_notify(struct net_device *dev, struct net_bridge_port *p,
struct br_mdb_entry *entry, int type)
{
@@ -330,6 +366,9 @@ static void __br_mdb_notify(struct net_device *dev, struct net_bridge_port *p,
switchdev_port_obj_del(port_dev, &mdb.obj);
}
+ if (!p)
+ br_mdb_switchdev_host(dev, entry, type);
+
skb = nlmsg_new(rtnl_mdb_nlmsg_size(), GFP_ATOMIC);
if (!skb)
goto errout;
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 0531b41d1f2d..74b9d916a58b 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -345,6 +345,8 @@ static size_t switchdev_obj_size(const struct switchdev_obj *obj)
return sizeof(struct switchdev_obj_port_vlan);
case SWITCHDEV_OBJ_ID_PORT_MDB:
return sizeof(struct switchdev_obj_port_mdb);
+ case SWITCHDEV_OBJ_ID_HOST_MDB:
+ return sizeof(struct switchdev_obj_port_mdb);
default:
BUG();
}
--
2.14.1
^ permalink raw reply related
* [PATCH v2 rfc 1/8] net: bridge: Rename mglist to host_joined
From: Andrew Lunn @ 2017-09-05 23:35 UTC (permalink / raw)
To: netdev; +Cc: jiri, nikolay, Florian Fainelli, Vivien Didelot, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
The boolean mglist indicates the host has joined a particular
multicast group on the bridge interface. It is badly named, obscuring
what is means. Rename it.
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
net/bridge/br_input.c | 2 +-
net/bridge/br_mdb.c | 2 +-
net/bridge/br_multicast.c | 14 +++++++-------
net/bridge/br_private.h | 2 +-
4 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 7637f58c1226..0dd55a183391 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -179,7 +179,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
mdst = br_mdb_get(br, skb, vid);
if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
br_multicast_querier_exists(br, eth_hdr(skb))) {
- if ((mdst && mdst->mglist) ||
+ if ((mdst && mdst->host_joined) ||
br_multicast_is_router(br)) {
local_rcv = true;
br->dev->stats.multicast++;
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index ca01def49af0..71885e251988 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -654,7 +654,7 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
call_rcu_bh(&p->rcu, br_multicast_free_pg);
err = 0;
- if (!mp->ports && !mp->mglist &&
+ if (!mp->ports && !mp->host_joined &&
netif_running(br->dev))
mod_timer(&mp->timer, jiffies);
break;
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 8dc5c8d69bcd..c6b2b8d419e7 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -249,7 +249,7 @@ static void br_multicast_group_expired(unsigned long data)
if (!netif_running(br->dev) || timer_pending(&mp->timer))
goto out;
- mp->mglist = false;
+ mp->host_joined = false;
if (mp->ports)
goto out;
@@ -292,7 +292,7 @@ static void br_multicast_del_pg(struct net_bridge *br,
p->flags);
call_rcu_bh(&p->rcu, br_multicast_free_pg);
- if (!mp->ports && !mp->mglist &&
+ if (!mp->ports && !mp->host_joined &&
netif_running(br->dev))
mod_timer(&mp->timer, jiffies);
@@ -775,7 +775,7 @@ static int br_multicast_add_group(struct net_bridge *br,
goto err;
if (!port) {
- mp->mglist = true;
+ mp->host_joined = true;
mod_timer(&mp->timer, now + br->multicast_membership_interval);
goto out;
}
@@ -1451,7 +1451,7 @@ static int br_ip4_multicast_query(struct net_bridge *br,
max_delay *= br->multicast_last_member_count;
- if (mp->mglist &&
+ if (mp->host_joined &&
(timer_pending(&mp->timer) ?
time_after(mp->timer.expires, now + max_delay) :
try_to_del_timer_sync(&mp->timer) >= 0))
@@ -1535,7 +1535,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,
goto out;
max_delay *= br->multicast_last_member_count;
- if (mp->mglist &&
+ if (mp->host_joined &&
(timer_pending(&mp->timer) ?
time_after(mp->timer.expires, now + max_delay) :
try_to_del_timer_sync(&mp->timer) >= 0))
@@ -1596,7 +1596,7 @@ br_multicast_leave_group(struct net_bridge *br,
br_mdb_notify(br->dev, port, group, RTM_DELMDB,
p->flags);
- if (!mp->ports && !mp->mglist &&
+ if (!mp->ports && !mp->host_joined &&
netif_running(br->dev))
mod_timer(&mp->timer, jiffies);
}
@@ -1636,7 +1636,7 @@ br_multicast_leave_group(struct net_bridge *br,
br->multicast_last_member_interval;
if (!port) {
- if (mp->mglist &&
+ if (mp->host_joined &&
(timer_pending(&mp->timer) ?
time_after(mp->timer.expires, time) :
try_to_del_timer_sync(&mp->timer) >= 0)) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index fd9ee73e0a6d..e79e0611908d 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -202,7 +202,7 @@ struct net_bridge_mdb_entry
struct rcu_head rcu;
struct timer_list timer;
struct br_ip addr;
- bool mglist;
+ bool host_joined;
};
struct net_bridge_mdb_htable
--
2.14.1
^ permalink raw reply related
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Stephen Hemminger @ 2017-09-06 0:11 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, jiri, nikolay, Florian Fainelli, Vivien Didelot
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
On Wed, 6 Sep 2017 01:35:02 +0200
Andrew Lunn <andrew@lunn.ch> wrote:
> After the very useful feedback from Nikolay, i threw away what i had,
> and started again. To recap:
>
> The linux bridge supports IGMP snooping. It will listen to IGMP
> reports on bridge ports and keep track of which groups have been
> joined on an interface. It will then forward multicast based on this
> group membership.
>
> When the bridge adds or removed groups from an interface, it uses
> switchdev to request the hardware add an mdb to a port, so the
> hardware can perform the selective forwarding between ports.
>
> What is not covered by the current bridge code, is IGMP joins/leaves
> from the host on the brX interface. These are not reported via
> switchdev so that hardware knows the local host is interested in the
> multicast frames.
>
> Luckily, the bridge does track joins/leaves on the brX interface. The
> code is obfusticated, which is why i missed it with my first attempt.
> So the first patch tries to remove this obfustication. Currently,
> there is no notifications sent when the bridge interface joins a
> group. The second patch adds them. bridge monitor then shows
> joins/leaves in the same way as for other ports of the bridge.
>
> Then starts the work passing down to the hardware that the host has
> joined/left a group. The existing switchdev mdb object cannot be used,
> since the semantics are different. The existing
> SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
> group should be forwarded out that port of the switch. However here we
> require the exact opposite. We want multicast frames for the group
> received on the port to the forwarded to the host. Hence add a new
> object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
> forward to the host. This new object is then propagated through the
> DSA layers. No DSA driver changes should be needed, this should just
> work...
>
> Getting the frames to the bridge as requested turned up an issue or
> three. The offload_fwd_mark is not being set by DSA, so the bridge
> floods the received frames back to the switch ports, resulting in
> duplication since the hardware has already flooded the packet. Fixing
> that turned up an issue with the meaning of
> SWITCHDEV_ATTR_ID_PORT_PARENT_ID in DSA. A DSA fabric of three
> switches needs to look to the software bridge as a single
> switch. Otherwise the offload_fwd_mark does not work, and we get
> duplication on the non-ingress switch. But each switch returned a
> different value. And they were not unique.
>
> The third and last issue will be explained in a followup email.
>
> Open questions:
>
> Is sending notifications going to break userspace?
> Is this new switchdev object O.K. for the few non-DSA switches that exist?
> Is the SWITCHDEV_ATTR_ID_PORT_PARENT_ID change acceptable?
>
> Andrew
>
> Andrew Lunn (8):
> net: bridge: Rename mglist to host_joined
> net: bridge: Send notification when host join/leaves a group
> net: bridge: Add/del switchdev object on host join/leave
> net: dsa: slave: Handle switchdev host mdb add/del
> net: dsa: switch: handle host mdb add/remove
> net: dsa: switch: Don't add CPU port to an mdb by default
> net: dsa: set offload_fwd_mark on received packets
> net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID
>
> include/net/switchdev.h | 1 +
> net/bridge/br_input.c | 2 +-
> net/bridge/br_mdb.c | 50 +++++++++++++++++++++++++++++---
> net/bridge/br_multicast.c | 18 +++++++-----
> net/bridge/br_private.h | 2 +-
> net/dsa/dsa.c | 1 +
> net/dsa/dsa_priv.h | 7 +++++
> net/dsa/port.c | 26 +++++++++++++++++
> net/dsa/slave.c | 16 ++++++++---
> net/dsa/switch.c | 72 +++++++++++++++++++++++++++++++++++++++--------
> net/switchdev/switchdev.c | 2 ++
> 11 files changed, 168 insertions(+), 29 deletions(-)
>
This looks much cleaner. I don't have DSA hardware or infrastructure to look deeper.
^ permalink raw reply
* Re: [PATCH 2/3] security: bpf: Add eBPF LSM hooks and security field to eBPF map
From: Alexei Starovoitov @ 2017-09-06 0:39 UTC (permalink / raw)
To: Chenbo Feng
Cc: Chenbo Feng, Daniel Borkmann, linux-security-module,
Jeffrey Vander Stoep, netdev, SELinux, Lorenzo Colitti
In-Reply-To: <CAMOXUJnF=UoWM7xm-Uk-zyhA1N9exjtDf8Xt+_EF2Kj6YUV0OQ@mail.gmail.com>
On Tue, Sep 05, 2017 at 02:59:38PM -0700, Chenbo Feng wrote:
> On Thu, Aug 31, 2017 at 7:05 PM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Thu, Aug 31, 2017 at 01:56:34PM -0700, Chenbo Feng wrote:
> >> From: Chenbo Feng <fengc@google.com>
> >>
> >> Introduce a pointer into struct bpf_map to hold the security information
> >> about the map. The actual security struct varies based on the security
> >> models implemented. Place the LSM hooks before each of the unrestricted
> >> eBPF operations, the map_update_elem and map_delete_elem operations are
> >> checked by security_map_modify. The map_lookup_elem and map_get_next_key
> >> operations are checked by securtiy_map_read.
> >>
> >> Signed-off-by: Chenbo Feng <fengc@google.com>
> >
> > ...
> >
> >> @@ -410,6 +418,10 @@ static int map_lookup_elem(union bpf_attr *attr)
> >> if (IS_ERR(map))
> >> return PTR_ERR(map);
> >>
> >> + err = security_map_read(map);
> >> + if (err)
> >> + return -EACCES;
> >> +
> >> key = memdup_user(ukey, map->key_size);
> >> if (IS_ERR(key)) {
> >> err = PTR_ERR(key);
> >> @@ -490,6 +502,10 @@ static int map_update_elem(union bpf_attr *attr)
> >> if (IS_ERR(map))
> >> return PTR_ERR(map);
> >>
> >> + err = security_map_modify(map);
> >
> > I don't feel these extra hooks are really thought through.
> > With such hook you'll disallow map_update for given map. That's it.
> > The key/values etc won't be used in such security decision.
> > In such case you don't need such hooks in update/lookup at all.
> > Only in map_creation and object_get calls where FD can be received.
> > In other words I suggest to follow standard unix practices:
> > Do permissions checks in open() and allow read/write() if FD is valid.
> > Same here. Do permission checks in prog_load/map_create/obj_pin/get
> > and that will be enough to jail bpf subsystem.
> > bpf cmds that need to be fast (like lookup and update) should not
> > have security hooks.
> >
> Thanks for pointing out this. I agree we should only do checks on
> creating and passing
> the object from one processes to another. And if we can still
> distinguish the read/write operation
> when obtaining the file fd, that would be great. But that may require
> us to add a new mode
> field into bpf_map struct and change the attribute struct when doing
> the bpf syscall. How do you
> think about this approach? Or we can do simple checks for
> bpf_obj_create and bpf_obj_use when
> creating the object and passing the object respectively but this
> solution cannot distinguish map modify and
> read.
iirc the idea to have read only maps was brought up in the past
(unfortunately no one took a stab at implementing it), but imo
that's better then relying on lsm to provide such restriction
and more secure, since even if you disable map_update via lsm,
the user can craft a program just to udpate the map from it.
For bpffs we already test for inode_permission(inode, MAY_WRITE);
during BPF_OBJ_GET command and we can extend this facility further.
Also we can allow dropping 'write' permissions from the map
(internally implemented via flag in struct bpf_map), so
update/delete operations won't be allowed.
This flag will be checked by syscall during map_update/delete
and by the verifier if such read-only map is used by the program
being loaded, so map_update/helpers won't be allowed in
such program.
Would also be good to support read-only programs as well
(progs that are read-only from kernel state point of view)
They won't be able to modify packets, maps, etc.
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Andrew Lunn @ 2017-09-06 0:47 UTC (permalink / raw)
To: netdev, Florian Fainelli, Vivien Didelot, Woojung.Huh, jbe,
sean.wang, john
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
> The third and last issue will be explained in a followup email.
Hi DSA hackers
So there is the third issue. It affects just DSA, but it possible
affects all DSA drivers.
This patchset broken broadcast with the Marvell drivers. It could
break broadcast on others drivers as well.
What i found is that the Marvell chips don't flood broadcast frames
between bridged ports. What appears to happen is there is a fdb miss,
so it gets forwarded to the CPU port for the host to deal with. The
software bridge when floods it out all ports of the bridge.
But the set offload_fwd_mark patch changes this. The software bridge
now assumes the hardware has already flooded broadcast out all ports
of the switch as needed. So it does not do any flooding itself. As a
result, on Marvell devices, broadcast packets don't get flooded at
all.
The issue can be fixed. I just need to add an mdb entry for the
broadcast address to each port of the bridge in the switch, and the
CPU port. But i don't know at what level to do this.
Should this be done at the DSA level, or at the driver level? Do any
chips do broadcast flooding in hardware already? Hence they currently
see broadcast duplication? If i add a broadcast mdb at the DSA level,
and the chip is already hard wired to flooding broadcast, is it going
to double flood?
Andrew
^ permalink raw reply
* Re: [PATCH net-next v6 3/3] openvswitch: enable NSH support
From: Yang, Yi @ 2017-09-06 0:53 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, e@erig.me
In-Reply-To: <878thtmgra.fsf-tFNcAqjVMyqKXQKiL6tip0B+6BGkLq7r@public.gmane.org>
On Tue, Sep 05, 2017 at 09:12:09PM +0800, Hannes Frederic Sowa wrote:
> "Yang, Yi" <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> writes:
>
> > We can change this later if we really find a better way to handle this
> > because it isn't defined in include/uapi/linux/openvswitch.h, so I still
> > have backdoor to do this if needed :-)
>
> Sorry, I can't follow you.
>
> It doesn't matter if something is defined in uapi headers, the
> observable behavior matters. If you allow users to configure flows with
> specific fields, it should not stop working at a future point in time.
Anyway this can be changed if it is really needed, so far current way is
the best one we can come up with, we would like to use it if you can
have better proposal. We have explained repeatedly context headers must
be matched and set, this is bottom line.
>
> > For our sfc use case in Opendaylight, we use context[0] for tunnel ID,
> > context[1] for destination IP for reverse RSP, they are used to match
> > and set in OpenFlow table, you can't limit users to use them in such
> > ways.
>
> So in your specific case you expect the masks to be completely stable
> because you defined a protocol on top of NSH, understood. And that is
> stable accross all possible paths. Understood as well.
>
> > If you check GENEVE implementation, tun_metadata* can be set or matched
> > as any other match field.
>
> Yes, I wrote that in my previous mail. I wonder why NSH context metadata
> is not in tun_metadata as well?
tun_metadata is tunnel metadata, GENEVE needs tunnel port, but NSH is
not so, NSH can't directly use tun_metadata, for MD type 2, we need to a
lot of rework on tun_metadata to make it shared between GENEVE and NSH,
I don't think this can happen in near term. So tun_metadata isn't option
for this now.
>
> > Actually the most important information in NSH are just these context
> > headers, you can't limit imagination of users by removing them from flow
> > keys.
> >
> > My point is to bring miniflow into kernel data path to fix your concern,
> > this will benefit your employer directly :-)
>
> Okay, interesting. It will probably not help if you still have a hash of
> a packet inside the flow table and use that for load balancing.
>
> [...]
>
> BTW I don't want to stop this patch, I am merely interested in how the
> bigger picture will look like in the end.
>
> Bye,
> Hannes
^ permalink raw reply
* [net 0/2][pull request] Intel Wired LAN Driver Updates 2017-09-05
From: Jeff Kirsher @ 2017-09-06 1:04 UTC (permalink / raw)
To: davem; +Cc: Jeff Kirsher, netdev, nhorman, sassmann, jogreene
This series contains fixes for i40e only.
These two patches fix an issue where our nvmupdate tool does not work on RHEL 7.4
and newer kernels, in fact, the use of the nvmupdate tool on newer kernels can
cause the cards to be non-functional unless these patches are applied.
Anjali reworks the locking around accessing the NVM so that NVM acquire timeouts
do not occur which was causing the failed firmware updates.
Jake correctly updates the wb_desc when reading the NVM through the AdminQ.
The following are changes since commit 6d9c153a0b84392406bc77600aa7d3ea365de041:
net: dsa: loop: Do not unregister invalid fixed PHY
and are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net-queue 40GbE
Anjali Singhai Jain (1):
i40e: avoid NVM acquire deadlock during NVM update
Jacob Keller (1):
i40e: point wb_desc at the nvm_wb_desc during i40e_read_nvm_aq
drivers/net/ethernet/intel/i40e/i40e_nvm.c | 99 +++++++++++++++---------
drivers/net/ethernet/intel/i40e/i40e_prototype.h | 2 -
2 files changed, 61 insertions(+), 40 deletions(-)
--
2.14.1
^ permalink raw reply
* [net 1/2] i40e: avoid NVM acquire deadlock during NVM update
From: Jeff Kirsher @ 2017-09-06 1:04 UTC (permalink / raw)
To: davem
Cc: Anjali Singhai Jain, netdev, nhorman, sassmann, jogreene,
Jacob Keller, Jeff Kirsher
In-Reply-To: <20170906010418.39007-1-jeffrey.t.kirsher@intel.com>
From: Anjali Singhai Jain <anjali.singhai@intel.com>
X722 devices use the AdminQ to access the NVM, and this requires taking
the AdminQ lock. Because of this, we lock the AdminQ during
i40e_read_nvm(), which is also called in places where the lock is
already held, such as the firmware update path which wants to lock once
and then unlock when finished after performing several tasks.
Although this should have only affected X722 devices, commit
96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices",
2016-12-02) added locking for all NVM reads, regardless of device
family.
This resulted in us accidentally causing NVM acquire timeouts on all
devices, causing failed firmware updates which left the eeprom in
a corrupt state.
Create unsafe non-locked variants of i40e_read_nvm_word and
i40e_read_nvm_buffer, __i40e_read_nvm_word and __i40e_read_nvm_buffer
respectively. These variants will not take the NVM lock and are expected
to only be called in places where the NVM lock is already held if
needed.
Since the only caller of i40e_read_nvm_buffer() was in such a path,
remove it entirely in favor of the unsafe version. If necessary we can
always add it back in the future.
Additionally, we now need to hold the NVM lock in i40e_validate_checksum
because the call to i40e_calc_nvm_checksum now assumes that the NVM lock
is held. We can further move the call to read I40E_SR_SW_CHECKSUM_WORD
up a bit so that we do not need to acquire the NVM lock twice.
This should resolve firmware updates and also fix potential raise that
could have caused the driver to report an invalid NVM checksum upon
driver load.
Reported-by: Stefan Assmann <sassmann@kpanic.de>
Fixes: 96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices", 2016-12-02)
Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_nvm.c | 98 +++++++++++++++---------
drivers/net/ethernet/intel/i40e/i40e_prototype.h | 2 -
2 files changed, 60 insertions(+), 40 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_nvm.c b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
index 800bd55d0159..154ba49c2c6f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_nvm.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
@@ -266,7 +266,7 @@ static i40e_status i40e_read_nvm_aq(struct i40e_hw *hw, u8 module_pointer,
* @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)
* @data: word read from the Shadow RAM
*
- * Reads one 16 bit word from the Shadow RAM using the GLNVM_SRCTL register.
+ * Reads one 16 bit word from the Shadow RAM using the AdminQ
**/
static i40e_status i40e_read_nvm_word_aq(struct i40e_hw *hw, u16 offset,
u16 *data)
@@ -280,27 +280,49 @@ static i40e_status i40e_read_nvm_word_aq(struct i40e_hw *hw, u16 offset,
}
/**
- * i40e_read_nvm_word - Reads Shadow RAM
+ * __i40e_read_nvm_word - Reads nvm word, assumes called does the locking
* @hw: pointer to the HW structure
* @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)
* @data: word read from the Shadow RAM
*
- * Reads one 16 bit word from the Shadow RAM using the GLNVM_SRCTL register.
+ * Reads one 16 bit word from the Shadow RAM.
+ *
+ * Do not use this function except in cases where the nvm lock is already
+ * taken via i40e_acquire_nvm().
+ **/
+static i40e_status __i40e_read_nvm_word(struct i40e_hw *hw,
+ u16 offset, u16 *data)
+{
+ i40e_status ret_code = 0;
+
+ if (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE)
+ ret_code = i40e_read_nvm_word_aq(hw, offset, data);
+ else
+ ret_code = i40e_read_nvm_word_srctl(hw, offset, data);
+ return ret_code;
+}
+
+/**
+ * i40e_read_nvm_word - Reads nvm word and acquire lock if necessary
+ * @hw: pointer to the HW structure
+ * @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)
+ * @data: word read from the Shadow RAM
+ *
+ * Reads one 16 bit word from the Shadow RAM.
**/
i40e_status i40e_read_nvm_word(struct i40e_hw *hw, u16 offset,
u16 *data)
{
- enum i40e_status_code ret_code = 0;
+ i40e_status ret_code = 0;
ret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
- if (!ret_code) {
- if (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE) {
- ret_code = i40e_read_nvm_word_aq(hw, offset, data);
- } else {
- ret_code = i40e_read_nvm_word_srctl(hw, offset, data);
- }
- i40e_release_nvm(hw);
- }
+ if (ret_code)
+ return ret_code;
+
+ ret_code = __i40e_read_nvm_word(hw, offset, data);
+
+ i40e_release_nvm(hw);
+
return ret_code;
}
@@ -393,31 +415,25 @@ static i40e_status i40e_read_nvm_buffer_aq(struct i40e_hw *hw, u16 offset,
}
/**
- * i40e_read_nvm_buffer - Reads Shadow RAM buffer
+ * __i40e_read_nvm_buffer - Reads nvm buffer, caller must acquire lock
* @hw: pointer to the HW structure
* @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF).
* @words: (in) number of words to read; (out) number of words actually read
* @data: words read from the Shadow RAM
*
* Reads 16 bit words (data buffer) from the SR using the i40e_read_nvm_srrd()
- * method. The buffer read is preceded by the NVM ownership take
- * and followed by the release.
+ * method.
**/
-i40e_status i40e_read_nvm_buffer(struct i40e_hw *hw, u16 offset,
- u16 *words, u16 *data)
+static i40e_status __i40e_read_nvm_buffer(struct i40e_hw *hw,
+ u16 offset, u16 *words,
+ u16 *data)
{
- enum i40e_status_code ret_code = 0;
+ i40e_status ret_code = 0;
- if (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE) {
- ret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
- if (!ret_code) {
- ret_code = i40e_read_nvm_buffer_aq(hw, offset, words,
- data);
- i40e_release_nvm(hw);
- }
- } else {
+ if (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE)
+ ret_code = i40e_read_nvm_buffer_aq(hw, offset, words, data);
+ else
ret_code = i40e_read_nvm_buffer_srctl(hw, offset, words, data);
- }
return ret_code;
}
@@ -499,15 +515,15 @@ static i40e_status i40e_calc_nvm_checksum(struct i40e_hw *hw,
data = (u16 *)vmem.va;
/* read pointer to VPD area */
- ret_code = i40e_read_nvm_word(hw, I40E_SR_VPD_PTR, &vpd_module);
+ ret_code = __i40e_read_nvm_word(hw, I40E_SR_VPD_PTR, &vpd_module);
if (ret_code) {
ret_code = I40E_ERR_NVM_CHECKSUM;
goto i40e_calc_nvm_checksum_exit;
}
/* read pointer to PCIe Alt Auto-load module */
- ret_code = i40e_read_nvm_word(hw, I40E_SR_PCIE_ALT_AUTO_LOAD_PTR,
- &pcie_alt_module);
+ ret_code = __i40e_read_nvm_word(hw, I40E_SR_PCIE_ALT_AUTO_LOAD_PTR,
+ &pcie_alt_module);
if (ret_code) {
ret_code = I40E_ERR_NVM_CHECKSUM;
goto i40e_calc_nvm_checksum_exit;
@@ -521,7 +537,7 @@ static i40e_status i40e_calc_nvm_checksum(struct i40e_hw *hw,
if ((i % I40E_SR_SECTOR_SIZE_IN_WORDS) == 0) {
u16 words = I40E_SR_SECTOR_SIZE_IN_WORDS;
- ret_code = i40e_read_nvm_buffer(hw, i, &words, data);
+ ret_code = __i40e_read_nvm_buffer(hw, i, &words, data);
if (ret_code) {
ret_code = I40E_ERR_NVM_CHECKSUM;
goto i40e_calc_nvm_checksum_exit;
@@ -593,14 +609,19 @@ i40e_status i40e_validate_nvm_checksum(struct i40e_hw *hw,
u16 checksum_sr = 0;
u16 checksum_local = 0;
+ /* We must acquire the NVM lock in order to correctly synchronize the
+ * NVM accesses across multiple PFs. Without doing so it is possible
+ * for one of the PFs to read invalid data potentially indicating that
+ * the checksum is invalid.
+ */
+ ret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
+ if (ret_code)
+ return ret_code;
ret_code = i40e_calc_nvm_checksum(hw, &checksum_local);
+ __i40e_read_nvm_word(hw, I40E_SR_SW_CHECKSUM_WORD, &checksum_sr);
+ i40e_release_nvm(hw);
if (ret_code)
- goto i40e_validate_nvm_checksum_exit;
-
- /* Do not use i40e_read_nvm_word() because we do not want to take
- * the synchronization semaphores twice here.
- */
- i40e_read_nvm_word(hw, I40E_SR_SW_CHECKSUM_WORD, &checksum_sr);
+ return ret_code;
/* Verify read checksum from EEPROM is the same as
* calculated checksum
@@ -612,7 +633,6 @@ i40e_status i40e_validate_nvm_checksum(struct i40e_hw *hw,
if (checksum)
*checksum = checksum_local;
-i40e_validate_nvm_checksum_exit:
return ret_code;
}
@@ -997,6 +1017,7 @@ static i40e_status i40e_nvmupd_state_writing(struct i40e_hw *hw,
break;
case I40E_NVMUPD_CSUM_CON:
+ /* Assumes the caller has acquired the nvm */
status = i40e_update_nvm_checksum(hw);
if (status) {
*perrno = hw->aq.asq_last_status ?
@@ -1011,6 +1032,7 @@ static i40e_status i40e_nvmupd_state_writing(struct i40e_hw *hw,
break;
case I40E_NVMUPD_CSUM_LCB:
+ /* Assumes the caller has acquired the nvm */
status = i40e_update_nvm_checksum(hw);
if (status) {
*perrno = hw->aq.asq_last_status ?
diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
index df613ea40313..a39b13197891 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
@@ -311,8 +311,6 @@ i40e_status i40e_acquire_nvm(struct i40e_hw *hw,
void i40e_release_nvm(struct i40e_hw *hw);
i40e_status i40e_read_nvm_word(struct i40e_hw *hw, u16 offset,
u16 *data);
-i40e_status i40e_read_nvm_buffer(struct i40e_hw *hw, u16 offset,
- u16 *words, u16 *data);
i40e_status i40e_update_nvm_checksum(struct i40e_hw *hw);
i40e_status i40e_validate_nvm_checksum(struct i40e_hw *hw,
u16 *checksum);
--
2.14.1
^ permalink raw reply related
* [net 2/2] i40e: point wb_desc at the nvm_wb_desc during i40e_read_nvm_aq
From: Jeff Kirsher @ 2017-09-06 1:04 UTC (permalink / raw)
To: davem; +Cc: Jacob Keller, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20170906010418.39007-1-jeffrey.t.kirsher@intel.com>
From: Jacob Keller <jacob.e.keller@intel.com>
When introducing the functions to read the NVM through the AdminQ, we
did not correctly mark the wb_desc.
Fixes: 7073f46e443e ("i40e: Add AQ commands for NVM Update for X722", 2015-06-05)
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_nvm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_nvm.c b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
index 154ba49c2c6f..26d7e9fe6220 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_nvm.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
@@ -230,6 +230,7 @@ static i40e_status i40e_read_nvm_aq(struct i40e_hw *hw, u8 module_pointer,
struct i40e_asq_cmd_details cmd_details;
memset(&cmd_details, 0, sizeof(cmd_details));
+ cmd_details.wb_desc = &hw->nvm_wb_desc;
/* Here we are checking the SR limit only for the flat memory model.
* We cannot do it for the module-based model, as we did not acquire
--
2.14.1
^ permalink raw reply related
* Re: [net 0/2][pull request] Intel Wired LAN Driver Updates 2017-09-05
From: David Miller @ 2017-09-06 3:03 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev, nhorman, sassmann, jogreene
In-Reply-To: <20170906010418.39007-1-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Tue, 5 Sep 2017 18:04:16 -0700
> This series contains fixes for i40e only.
Pulled, thanks Jeff.
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Alexander Duyck @ 2017-09-06 3:06 UTC (permalink / raw)
To: Tom Herbert
Cc: Hannes Frederic Sowa, Saeed Mahameed, Saeed Mahameed,
David S. Miller, Linux Netdev List
In-Reply-To: <CALx6S3698iBwLX3wXw8_rhHbhztLh-BAkkhn68=z=2FgbfTkZg@mail.gmail.com>
On Tue, Sep 5, 2017 at 2:13 PM, Tom Herbert <tom@herbertland.com> wrote:
>> The situation with encapsulation is even more complicated:
>>
>> We are basically only interested in the UDP/vxlan/Ethernet/IP/UDP
>> constellation. If we do the fragmentation inside the vxlan tunnel and
>> carry over the skb hash to all resulting UDP/vxlan packets source ports,
>> we are fine and reordering on the receiver NIC won't happen in this
>> case. If the fragmentation happens on the outer UDP header, this will
>> result in reordering of the inner L2 flow. Unfortunately this depends on
>> how the vxlan tunnel was set up, how other devices do that and (I
>> believe so) on the kernel version.
>>
> This really isn't that complicated. The assumption that an IP network
> always delivers packets in order is simply wrong. The inventors of
> VXLAN must have know full well that when you use IP, packets can and
> eventually will be delivered out of order. This isn't just because of
> fragmentation, there are many other reasons that packets can be
> delivered OOO. This also must have been known when IP/GRE and any
> other protocol that carries L2 over IP was invented. If OOO is an
> issue for these protocols then they need to be fixed-- this is not a
> concern with IP protocol nor the stack.
>
> Tom
As far as a little background on the original patch I believe the
issue that was fixed by the patch was a video streaming application
that was sending/receiving a mix of fragmented and non-fragmented
packets. Receiving them out of order due to the fragmentation was
causing issues with stutters in the video and so we ended up disabling
UDP by default in the NICs listed. We decided to go that way as UDP
RSS was viewed as a performance optimization, while the out-of-order
problems were viewed as a functionality issue.
The default for i40e is to have UDP RSS hashing enabled if I recall
correctly. Basically as we move away from enterprise to cloud I
suspect that is going to be the case more and more since all the UDP
tunnels require either port recognition or UDP RSS to be enabled. For
now we carry the ability to enable UDP RSS if desired in the legacy
drivers, and I believe we have some white papers somewhere that
suggest enabling it if you are planning to use UDP based tunnels.
- Alex
^ permalink raw reply
* Re: [PATCH v3 2/2] ip6_tunnel: fix ip6 tunnel lookup in collect_md mode
From: Alexei Starovoitov @ 2017-09-06 3:14 UTC (permalink / raw)
To: Haishuang Yan, David S. Miller; +Cc: netdev, linux-kernel
In-Reply-To: <1504514174-14958-2-git-send-email-yanhaishuang@cmss.chinamobile.com>
On 9/4/17 1:36 AM, Haishuang Yan wrote:
> In collect_md mode, if the tun dev is down, it still can call
> __ip6_tnl_rcv to receive on packets, and the rx statistics increase
> improperly.
>
> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
> Cc: Alexei Starovoitov <ast@fb.com>
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
>
> ---
> Change since v3:
> * Increment rx_dropped if tunnel device is not up, suggested by
> Pravin B Shelar
> * Fix wrong recipient address
> ---
> net/ipv6/ip6_tunnel.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 10a693a..e91d3b6 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -171,8 +171,11 @@ static struct net_device_stats *ip6_get_stats(struct net_device *dev)
> }
>
> t = rcu_dereference(ip6n->collect_md_tun);
> - if (t)
> - return t;
> + if (t) {
> + if (t->dev->flags & IFF_UP)
> + return t;
> + t->dev->stats.rx_dropped++;
> + }
Why increment the stats only for this drop case?
There are plenty of other conditions where packet
will be dropped in ip6 tunnel. I think it's important
to present consistent behavior to the users,
so I'd increment drop stats either for all drop cases
or for none. And today it's none.
The ! IFF_UP case should probably be return NULL too.
^ 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