* Re: [PATCH net v2] rds: Fix incorrect statistics counting
From: Santosh Shilimkar @ 2017-09-06 16:48 UTC (permalink / raw)
To: Håkon Bugge, David S . Miller
Cc: netdev, linux-rdma, rds-devel, linux-kernel, knut.omang
In-Reply-To: <20170906163551.20387-1-Haakon.Bugge@oracle.com>
On 9/6/2017 9:35 AM, Håkon Bugge wrote:
> In rds_send_xmit() there is logic to batch the sends. However, if
> another thread has acquired the lock and has incremented the send_gen,
> it is considered a race and we yield. The code incrementing the
> s_send_lock_queue_raced statistics counter did not count this event
> correctly.
>
> This commit counts the race condition correctly.
>
> Changes from v1:
> - Removed check for *someone_on_xmit()*
> - Fixed incorrect indentation
>
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Reviewed-by: Knut Omang <knut.omang@oracle.com>
> ---
Thanks for the update.
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
^ permalink raw reply
* RE: [PATCH] DSA support for Micrel KSZ8895
From: Tristram.Ha @ 2017-09-06 16:47 UTC (permalink / raw)
To: muvarov
Cc: pavel, Woojung.Huh, nathan.leigh.conrad, vivien.didelot,
f.fainelli, netdev, linux-kernel, andrew
In-Reply-To: <CAJGZr0+WueUjZJTLmcJRXtLtgUTUGnPCgiHQir=v5cRf3ido_g@mail.gmail.com>
> -----Original Message-----
> From: Maxim Uvarov [mailto:muvarov@gmail.com]
> Sent: Wednesday, September 06, 2017 2:15 AM
> To: Tristram Ha - C24268
> Cc: Pavel Machek; Woojung Huh - C21699; Nathan Conrad; Vivien Didelot;
> Florian Fainelli; netdev; linux-kernel@vger.kernel.org; Andrew Lunn
> Subject: Re: [PATCH] DSA support for Micrel KSZ8895
>
> 2017-08-31 0:32 GMT+03:00 <Tristram.Ha@microchip.com>:
> >> On Mon 2017-08-28 16:09:27, Andrew Lunn wrote:
> >> > > I may be confused here, but AFAICT:
> >> > >
> >> > > 1) Yes, it has standard layout when accessed over MDIO.
> >> >
> >> >
> >> > Section 4.8 of the datasheet says:
> >> >
> >> > All the registers defined in this section can be also accessed
> >> > via the SPI interface.
> >> >
> >> > Meaning all PHY registers can be access via the SPI interface. So
> >> > you should be able to make a standard Linux MDIO bus driver which
> >> > performs SPI reads.
> >>
> >> As far as I can tell (and their driver confirms) -- yes, all those
> >> registers can be accessed over the SPI, they are just shuffled
> >> around... hence MDIO emulation code. I copied it from their code (see
> >> the copyrights) so no, I don't believe there's nicer solution.
> >>
> >> Best regards,
> >>
> >>
> >> Pavel
> >
> > Can you hold on your developing work on KSZ8895 driver? I am afraid your
> effort may be in vain. We at Microchip are planning to release DSA drivers
> for all KSZ switches, starting at KSZ8795, then KSZ8895, and KSZ8863.
> >
> > The driver files all follow the structures of the current KSZ9477 DSA driver,
> and the file tag_ksz.c will be updated to handle the tail tag of different chips,
> which requires including the ksz_priv.h header. That is required
> nevertheless to support using the offload_fwd_mark indication.
> >
> > The KSZ8795 driver will be submitted after Labor Day (9/4) if testing reveals
> no problem. The KSZ8895 driver will be submitted right after that. You
> should have no problem using the driver right away.
> >
>
> Hello Tristram, is there any update for that driver?
>
> Maxim.
>
The patches are under review internally and will need to be updated and approved by Woojung before formal submission. Problem is although KSZ8795 and KSZ8895 drivers are new code and will be submitted as RFC, they depend on the change of KSZ9477 driver currently in the kernel, which require more rigorous review.
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Andrew Lunn @ 2017-09-06 16:42 UTC (permalink / raw)
To: Florian Fainelli
Cc: Roopa Prabhu, netdev, Vivien Didelot, Woojung.Huh, jbe, sean.wang,
john
In-Reply-To: <9ECEF4E4-A39B-4578-8BDC-7842D20F3C81@gmail.com>
> >On the switch asics we work with, the driver has information if the
> >packet was
> >forwarded in hardware. This is per packet reason code telling why the
> >CPU is seeing the packet.
> >The driver can use this information to reset skb->offload_fwd_mark to
> >allow software forward.
> I am not positive this is universally available across different
> switch vendors.
It is not universally available. We cannot rely on it being available
with switches supported by DSA.
We have a few choices:
1) We assume anything the switch forwards to the CPU has also been
sent out whatever ports of the switch it needs to. Set
offload_fwd_mark.
2) We assume anything the switch forwards to the CPU has not gone
anywhere else, and the bridge needs to send it out whatever ports
it thinks. Don't set offload_fwd_mark.
3) We define some rules about what packets the switch should handle,
and then do some deep packet inspection to decide if
offload_fwd_mark should be set or not.
I don't see 3) being possible. We are dealing with a fixed silicon
data path, not something which is fully programmable.
So it is down to 1) or 2). I've been assuming 1), but maybe we need to
discuss that as well.
Andrew
^ permalink raw reply
* [PATCH net v2] rds: Fix incorrect statistics counting
From: Håkon Bugge @ 2017-09-06 16:35 UTC (permalink / raw)
To: Santosh Shilimkar, David S . Miller
Cc: netdev, linux-rdma, rds-devel, linux-kernel, knut.omang
In rds_send_xmit() there is logic to batch the sends. However, if
another thread has acquired the lock and has incremented the send_gen,
it is considered a race and we yield. The code incrementing the
s_send_lock_queue_raced statistics counter did not count this event
correctly.
This commit counts the race condition correctly.
Changes from v1:
- Removed check for *someone_on_xmit()*
- Fixed incorrect indentation
Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
Reviewed-by: Knut Omang <knut.omang@oracle.com>
---
net/rds/send.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/rds/send.c b/net/rds/send.c
index 058a407..b52cdc8 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -428,14 +428,18 @@ int rds_send_xmit(struct rds_conn_path *cp)
* some work and we will skip our goto
*/
if (ret == 0) {
+ bool raced;
+
smp_mb();
+ raced = send_gen != READ_ONCE(cp->cp_send_gen);
+
if ((test_bit(0, &conn->c_map_queued) ||
- !list_empty(&cp->cp_send_queue)) &&
- send_gen == READ_ONCE(cp->cp_send_gen)) {
- rds_stats_inc(s_send_lock_queue_raced);
+ !list_empty(&cp->cp_send_queue)) && !raced) {
if (batch_count < send_batch_count)
goto restart;
queue_delayed_work(rds_wq, &cp->cp_send_w, 1);
+ } else if (raced) {
+ rds_stats_inc(s_send_lock_queue_raced);
}
}
out:
--
2.9.3
^ permalink raw reply related
* Re: [PATCH net-next 1/1] hv_netvsc: fix deadlock on hotplug
From: Stephen Hemminger @ 2017-09-06 16:36 UTC (permalink / raw)
To: Haiyang Zhang
Cc: devel@linuxdriverproject.org, Stephen Hemminger,
netdev@vger.kernel.org
In-Reply-To: <DM5PR21MB0475451095CC657BB54C4E50CA970@DM5PR21MB0475.namprd21.prod.outlook.com>
On Wed, 6 Sep 2017 16:23:45 +0000
Haiyang Zhang <haiyangz@microsoft.com> wrote:
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Wednesday, September 6, 2017 11:19 AM
> > To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>
> > Cc: devel@linuxdriverproject.org; netdev@vger.kernel.org
> > Subject: [PATCH net-next 1/1] hv_netvsc: fix deadlock on hotplug
> >
> > When a virtual device is added dynamically (via host console), then
> > the vmbus sends an offer message for the primary channel. The processing
> > of this message for networking causes the network device to then
> > initialize the sub channels.
> >
> > The problem is that setting up the sub channels needs to wait until
> > the subsequent subchannel offers have been processed. These offers
> > come in on the same ring buffer and work queue as where the primary
> > offer is being processed; leading to a deadlock.
> >
> > This did not happen in older kernels, because the sub channel waiting
> > logic was broken (it wasn't really waiting).
> >
> > The solution is to do the sub channel setup in its own work queue
> > context that is scheduled by the primary channel setup; and then
> > happens later.
> >
> > Fixes: 732e49850c5e ("netvsc: fix race on sub channel creation")
> > Reported-by: Dexuan Cui <decui@microsoft.com>
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> > Should also go to stable, but this version does not apply cleanly
> > to 4.13. Have another patch for that.
> >
> > drivers/net/hyperv/hyperv_net.h | 1 +
> > drivers/net/hyperv/netvsc_drv.c | 8 +--
> > drivers/net/hyperv/rndis_filter.c | 106 ++++++++++++++++++++++++++-----
> > -------
> > 3 files changed, 74 insertions(+), 41 deletions(-)
>
> The patch looks overall. I just have a question:
>
> With this patch, after module load and probe is done, there may still be
> subchannels being processed. If rmmod immediately, the subchannel offers
> may hit half-way removed device structures... Do we also need to add
> cancel_work_sync(&dev->subchan_work) to the top of netvsc_remove()?
>
> unregister_netdevice() includes device close, but it's only called later
> in the netvsc_remove() when rndis is already removed.
>
> Thanks,
> - Haiyang
Good catch.
If the driver called unregister_netdevice first before doing rndis_filter_device_remove
that would solve the problem. That wouldn't cause additional problems and it makes
sense to close the network layer first.
^ permalink raw reply
* Re: [PATCH v2 rfc 8/8] net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID
From: Andrew Lunn @ 2017-09-06 16:29 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Vivien Didelot, netdev, jiri, nikolay
In-Reply-To: <0D9EF30F-ED95-486E-91B0-E18D5E8689FE@gmail.com>
> >Yes, you are correct. I will change this.
>
> A switch cluster should be tied to the same dsa_switch_tree though,
> can we use dst->tree as an unique identifier?
Yes, that is what Vivien was suggesting.
Andrew
^ permalink raw reply
* Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP
From: Daniel Borkmann @ 2017-09-06 16:24 UTC (permalink / raw)
To: Jesper Dangaard Brouer, netdev, David S. Miller
Cc: John Fastabend, Andy Gospodarek
In-Reply-To: <150471158528.3727.12324542627400287360.stgit@firesoul>
On 09/06/2017 05:26 PM, Jesper Dangaard Brouer wrote:
> Using bpf_redirect_map is allowed for generic XDP programs, but the
> appropriate map lookup was never performed in xdp_do_generic_redirect().
>
> Instead the map-index is directly used as the ifindex. For the
Good point, but ...
[...]
> net/core/filter.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 5912c738a7b2..6a4745bf2c9f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
> }
> EXPORT_SYMBOL_GPL(xdp_do_redirect);
>
> +static int xdp_do_generic_redirect_map(struct net_device *dev,
> + struct sk_buff *skb,
> + struct bpf_prog *xdp_prog)
> +{
> + struct redirect_info *ri = this_cpu_ptr(&redirect_info);
> + struct bpf_map *map = ri->map;
> + u32 index = ri->ifindex;
> + struct net_device *fwd;
> + int err;
> +
> + ri->ifindex = 0;
> + ri->map = NULL;
> +
> + fwd = __dev_map_lookup_elem(map, index);
> + if (!fwd) {
> + err = -EINVAL;
> + goto err;
> + }
> + skb->dev = fwd;
> + _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
> + return 0;
> +err:
> + _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
> + return err;
> +}
> +
> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> struct bpf_prog *xdp_prog)
> {
> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> unsigned int len;
> int err = 0;
>
> + if (ri->map)
> + return xdp_do_generic_redirect_map(dev, skb, xdp_prog);
This is not quite correct. Really, the only thing you want
to do here is more or less ...
int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
struct bpf_prog *xdp_prog)
{
struct redirect_info *ri = this_cpu_ptr(&redirect_info);
struct bpf_map *map = ri->map;
u32 index = ri->ifindex;
struct net_device *fwd;
unsigned int len;
int err = 0;
ri->ifindex = 0;
ri->map = NULL;
if (map)
fwd = __dev_map_lookup_elem(map, index);
else
fwd = dev_get_by_index_rcu(dev_net(dev), index);
if (unlikely(!fwd)) {
err = -EINVAL;
goto err;
}
[...]
... such that you have a common path to also do the IFF_UP
and MTU checks that are done here, but otherwise omitted in
your patch.
Otherwise it looks good, but note that it also doesn't really
resolve the issue you mention wrt stale map pointers by the
way. This would need a different way to clear out the pointers
from redirect_info, I'm thinking when we have devmap dismantle
time after RCU grace period we should check whether there are
still stale pointers from this map around and clear them under
disabled preemption, but need to brainstorm a bit more on that
first.
> fwd = dev_get_by_index_rcu(dev_net(dev), index);
> ri->ifindex = 0;
> if (unlikely(!fwd)) {
>
^ permalink raw reply
* RE: [PATCH net-next 1/1] hv_netvsc: fix deadlock on hotplug
From: Haiyang Zhang @ 2017-09-06 16:23 UTC (permalink / raw)
To: Stephen Hemminger, KY Srinivasan, Stephen Hemminger
Cc: devel@linuxdriverproject.org, netdev@vger.kernel.org
In-Reply-To: <20170906151925.15221-2-sthemmin@microsoft.com>
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, September 6, 2017 11:19 AM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>
> Cc: devel@linuxdriverproject.org; netdev@vger.kernel.org
> Subject: [PATCH net-next 1/1] hv_netvsc: fix deadlock on hotplug
>
> When a virtual device is added dynamically (via host console), then
> the vmbus sends an offer message for the primary channel. The processing
> of this message for networking causes the network device to then
> initialize the sub channels.
>
> The problem is that setting up the sub channels needs to wait until
> the subsequent subchannel offers have been processed. These offers
> come in on the same ring buffer and work queue as where the primary
> offer is being processed; leading to a deadlock.
>
> This did not happen in older kernels, because the sub channel waiting
> logic was broken (it wasn't really waiting).
>
> The solution is to do the sub channel setup in its own work queue
> context that is scheduled by the primary channel setup; and then
> happens later.
>
> Fixes: 732e49850c5e ("netvsc: fix race on sub channel creation")
> Reported-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
> Should also go to stable, but this version does not apply cleanly
> to 4.13. Have another patch for that.
>
> drivers/net/hyperv/hyperv_net.h | 1 +
> drivers/net/hyperv/netvsc_drv.c | 8 +--
> drivers/net/hyperv/rndis_filter.c | 106 ++++++++++++++++++++++++++-----
> -------
> 3 files changed, 74 insertions(+), 41 deletions(-)
The patch looks overall. I just have a question:
With this patch, after module load and probe is done, there may still be
subchannels being processed. If rmmod immediately, the subchannel offers
may hit half-way removed device structures... Do we also need to add
cancel_work_sync(&dev->subchan_work) to the top of netvsc_remove()?
unregister_netdevice() includes device close, but it's only called later
in the netvsc_remove() when rndis is already removed.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH net] rds: Fix incorrect statistics counting
From: Santosh Shilimkar @ 2017-09-06 16:21 UTC (permalink / raw)
To: Håkon Bugge
Cc: David S . Miller, netdev, OFED mailing list, rds-devel,
linux-kernel, Knut Omang
In-Reply-To: <715EA84D-6ACA-45DE-9EA2-6122E11545E8@oracle.com>
On 9/6/2017 9:12 AM, Håkon Bugge wrote:
>
[...]
>
> Hi Santosh,
>
>
> Yes, I agree with accuracy of s_send_lock_queue_raced. But the main point is that the existing code counts some partial share of when it is _not_ raced.
>
> So, in the critical path, my patch adds one test_bit(), which hits the local CPU cache, if not raced. If raced, some other thread is in control, so I would not think the added cycles would make any big difference.
>
Cycles added for no good reason is the point.
> I can send a v2 where the race tightening is removed if you like.
>
Yes please.
Regards,
Santosh
^ permalink raw reply
* Re: [pull request][net-next 0/3] Mellanox, mlx5 GRE tunnel offloads
From: Tom Herbert @ 2017-09-06 16:17 UTC (permalink / raw)
To: Alexander Duyck
Cc: Hannes Frederic Sowa, Saeed Mahameed, Saeed Mahameed,
David S. Miller, Linux Netdev List
In-Reply-To: <CAKgT0UeHbs3cM=yvSAdQMN92mWZjaqsq5Pyk1SkC8V_DK8pf3A@mail.gmail.com>
On Tue, Sep 5, 2017 at 8:06 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> 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.
>
Hi Alex,
Thanks for the details! Were you able to find the root cause for this?
In particular, it would be interesting to know if it is the kernel or
device that introduced the jitter, or if it's the application that
doesn't handle OOO well...
Tom
^ permalink raw reply
* Re: [PATCH net] rds: Fix incorrect statistics counting
From: Håkon Bugge @ 2017-09-06 16:12 UTC (permalink / raw)
To: Santosh Shilimkar
Cc: David S . Miller, netdev, OFED mailing list, rds-devel,
linux-kernel, Knut Omang
In-Reply-To: <b620a15a-e60e-5c35-ffef-ee485d338774@oracle.com>
> On 6 Sep 2017, at 17:58, Santosh Shilimkar <santosh.shilimkar@oracle.com> wrote:
>
> On 9/6/2017 8:29 AM, Håkon Bugge wrote:
>> In rds_send_xmit() there is logic to batch the sends. However, if
>> another thread has acquired the lock, it is considered a race and we
>> yield. The code incrementing the s_send_lock_queue_raced statistics
>> counter did not count this event correctly.
>> This commit removes a small race in determining the race and
>> increments the statistics counter correctly.
>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>> Reviewed-by: Knut Omang <knut.omang@oracle.com>
>> ---
>> net/rds/send.c | 16 +++++++++++++---
>> 1 file changed, 13 insertions(+), 3 deletions(-)
> Those counters are not really to give that accurate so
> am not very keen to add additional cycles in send paths
> and add additional code. Have you seen any real issue
> or this is just a observation. s_send_lock_queue_raced
> counter is never used to check for smaller increments
> and hence the question.
Hi Santosh,
Yes, I agree with accuracy of s_send_lock_queue_raced. But the main point is that the existing code counts some partial share of when it is _not_ raced.
So, in the critical path, my patch adds one test_bit(), which hits the local CPU cache, if not raced. If raced, some other thread is in control, so I would not think the added cycles would make any big difference.
I can send a v2 where the race tightening is removed if you like.
Thxs, Håkon
^ permalink raw reply
* Re: [PATCH v2 rfc 8/8] net: dsa: Fix SWITCHDEV_ATTR_ID_PORT_PARENT_ID
From: Florian Fainelli @ 2017-09-06 16:09 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot; +Cc: netdev, jiri, nikolay
In-Reply-To: <20170906150825.GB15315@lunn.ch>
On September 6, 2017 8:08:25 AM PDT, Andrew Lunn <andrew@lunn.ch> wrote:
>> > 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.
>>
>> That is not correct. Support for multiple CPU ports is coming and in
>> this case, you can have two CPU host interfaces wired to two switch
>> ports of the same tree. So two different master MAC addresses.
>
>Yes, you are correct. I will change this.
A switch cluster should be tied to the same dsa_switch_tree though, can we use dst->tree as an unique identifier?
--
Florian
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Florian Fainelli @ 2017-09-06 16:06 UTC (permalink / raw)
To: Roopa Prabhu, Andrew Lunn
Cc: netdev, Vivien Didelot, Woojung.Huh, jbe, sean.wang, john
In-Reply-To: <CAJieiUj-ObfKAZa18JxnWSswLGK-mRLjqE7qyCrZ5tsVftf46w@mail.gmail.com>
On September 6, 2017 8:54:33 AM PDT, Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>On Tue, Sep 5, 2017 at 5:47 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>> 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?
>>
>
>On the switch asics we work with, the driver has information if the
>packet was
>forwarded in hardware. This is per packet reason code telling why the
>CPU is seeing the packet.
>The driver can use this information to reset skb->offload_fwd_mark to
>allow software forward.
I am not positive this is universally available across different switch vendors. In Broadcom tag (net/dsa/tag_brcm.c) the reason code definitely tells you that but it also largely depends on whether you have configured SW managed forwarding or not and that translates in having either the HW do all the address learning itself or having SW do it which is less desirable since you end-up with a possibility huge FDB of 4k entries to manage in SW.
--
Florian
^ permalink raw reply
* Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP
From: Jesper Dangaard Brouer @ 2017-09-06 16:01 UTC (permalink / raw)
To: Andy Gospodarek
Cc: netdev@vger.kernel.org, David S. Miller, John Fastabend, brouer
In-Reply-To: <CAHashqBPDDETq2qdj3n8uL3hE61f=a5cv_eF6a9apoRzNVqaxA@mail.gmail.com>
On Wed, 6 Sep 2017 11:44:18 -0400
Andy Gospodarek <andy@greyhouse.net> wrote:
> On Wed, Sep 6, 2017 at 11:26 AM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> > Using bpf_redirect_map is allowed for generic XDP programs, but the
> > appropriate map lookup was never performed in xdp_do_generic_redirect().
> >
> > Instead the map-index is directly used as the ifindex. For the
> > xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
> > sending on ifindex 0 which isn't valid, resulting in getting SKB
> > packets dropped. Thus, the reported performance numbers are wrong in
> > commit 24251c264798 ("samples/bpf: add option for native and skb mode
> > for redirect apps") for the 'xdp_redirect_map -S' case.
> >
> > It might seem innocent this was lacking, but it can actually crash the
> > kernel. The potential crash is caused by not consuming redirect_info->map.
> > The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map
> > pointer, which will survive even after unloading the xdp bpf_prog and
> > deallocating the devmap data-structure. This leaves a dead map
> > pointer around. The kernel will crash when loading the xdp_redirect
> > sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)
> > and returns XDP_REDIRECT, which will cause it to dereference the map
> > pointer.
>
> Nice catch!
>
> Since 'net-next' is closed and this is a bugfix it seems like this is
> a good candidate for 'net' right?
Yes, I know 'net-next' is closed, but 'net' doesn't contain the
XDP_REDIRECT code yet... thus I had to base it on net-next ;-)
> >
> > Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
> > Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
> Acked-by: Andy Gospodarek <andy@greyhouse.net>
Thanks
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net] rds: Fix incorrect statistics counting
From: Santosh Shilimkar @ 2017-09-06 15:58 UTC (permalink / raw)
To: Håkon Bugge, David S . Miller
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
rds-devel-N0ozoZBvEnrZJqsBc5GL+g,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
knut.omang-QHcLZuEGTsvQT0dZR+AlfA
In-Reply-To: <20170906152950.17766-1-Haakon.Bugge-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
On 9/6/2017 8:29 AM, Håkon Bugge wrote:
> In rds_send_xmit() there is logic to batch the sends. However, if
> another thread has acquired the lock, it is considered a race and we
> yield. The code incrementing the s_send_lock_queue_raced statistics
> counter did not count this event correctly.
>
> This commit removes a small race in determining the race and
> increments the statistics counter correctly.
>
> Signed-off-by: Håkon Bugge <haakon.bugge-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Knut Omang <knut.omang-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
> net/rds/send.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
Those counters are not really to give that accurate so
am not very keen to add additional cycles in send paths
and add additional code. Have you seen any real issue
or this is just a observation. s_send_lock_queue_raced
counter is never used to check for smaller increments
and hence the question.
Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Roopa Prabhu @ 2017-09-06 15:54 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, Florian Fainelli, Vivien Didelot, Woojung.Huh, jbe,
sean.wang, john
In-Reply-To: <20170906004703.GB27385@lunn.ch>
On Tue, Sep 5, 2017 at 5:47 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> 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?
>
On the switch asics we work with, the driver has information if the packet was
forwarded in hardware. This is per packet reason code telling why the
CPU is seeing the packet.
The driver can use this information to reset skb->offload_fwd_mark to
allow software forward.
^ permalink raw reply
* Re: [PATCH net] Revert "net: phy: Correctly process PHY_HALTED in phy_stop_machine()"
From: Mason @ 2017-09-06 15:51 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn
Cc: Marc Gonzalez, David Daney, netdev, Geert Uytterhoeven,
David Miller, Mans Rullgard, Thibaud Cornic
In-Reply-To: <7b1c1dc9-b6e3-a1bd-2e36-474946741a79@gmail.com>
On 31/08/2017 21:18, Florian Fainelli wrote:
> On 08/31/2017 12:09 PM, Mason wrote:
>
>> On 31/08/2017 19:03, Florian Fainelli wrote:
>>
>>> On 08/31/2017 05:29 AM, Marc Gonzalez wrote:
>>>
>>>> On 31/08/2017 02:49, Florian Fainelli wrote:
>>>>
>>>>> The original motivation for this change originated from Marc Gonzalez
>>>>> indicating that his network driver did not have its adjust_link callback
>>>>> executing with phydev->link = 0 while he was expecting it.
>>>>
>>>> I expect the core to call phy_adjust_link() for link changes.
>>>> This used to work back in 3.4 and was broken somewhere along
>>>> the way.
>>>
>>> If that was working correctly in 3.4 surely we can look at the diff and
>>> figure out what changed, even maybe find the offending commit, can you
>>> do that?
>>
>> Bisecting would a be a huge pain because my platform was
>> not upstream until v4.4
>
> Then just diff the file and try to pinpoint which commit may have
> changed that?
Running 'ip link set eth0 down' on the command-line.
In v3.4 => adjust_link() callback is called
In v4.5 => adjust_link() callback is NOT called
$ git log --oneline --no-merges v3.4..v4.5 drivers/net/phy/phy.c | wc -l
59
I'm not sure what "just diff the file" entails.
I can't move 3.4 up, nor move 4.5 down.
I'm not even sure the problem comes from drivers/net/phy/phy.c
to be honest.
Regards.
^ permalink raw reply
* RE: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Woojung.Huh @ 2017-09-06 15:49 UTC (permalink / raw)
To: andrew, netdev, f.fainelli, vivien.didelot, jbe, sean.wang, john
In-Reply-To: <20170906004703.GB27385@lunn.ch>
Andrew,
> 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.
Is this IGMP snooping enabled mode in Marvell chip?
^ permalink raw reply
* Re: VLAN/bridge "compression" in wifi (was: Re: [PATCH 3/8] qtnfmac: implement AP_VLAN iftype support)
From: Sergey Matyukevich @ 2017-09-06 15:45 UTC (permalink / raw)
To: Johannes Berg
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA, netdev, Igor Mitsyanko,
Avinash Patil
In-Reply-To: <1504621233.12380.21.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Hi Johannes and all,
> > In a way this feature seems mis-designed - you never have 802.1Q tags
> > over the air, but you're inserting them on RX and stripping them on
> > TX, probably in order to make bridging to ethernet easier and not
> > have to have 802.1Q acceleration on the ethernet port, or - well - in
> > order to have an ability to do this with an ethernet card that only
> > has a single CPU port.
>
> Ok this isn't really right either - it's only for saving the 802.1Q
> acceleration on the Ethernet port, really - and saving the extra
> bridges.
>
> To clarify, I think what you - conceptually - want is the following
> topology:
>
> +--- eth0.1 --- br.1 --- wlan0.1
> |
> eth0 ---+--- eth0.2 --- br.2 --- wlan0.2
> |
> +--- eth0.3 --- br.3 --- wlan0.3
>
> where eth0.N is just "ip link add link eth0 name eth0.N type vlan id N"
> and br.N is obviously a bridge for each, and the wlan0.N are AP_VLAN
> type interfaces that isolate the clients against each other as far as
> wifi is concerned.
>
> Is this correct? As far as I understand, that's the baseline topology
> that you're trying to achieve, expressed in terms of Linux networking.
That's right. In fact, hostapd is able to create this kind of network
bridge infrastructure automatically when it is built
with CONFIG_FULL_DYNAMIC_VLAN option enabled.
> Now, you seem to want to compress this to
>
> +--- wlan0.1
> |
> eth0 --- br ---+--- wlan0.2
> |
> +--- wlan0.3
>
> and have the 802.1Q tag insertion/removal that's normally configured to
> happen in eth0.N already be handled in wlan0.N.
>
> Also correct?
Exactly. And yes, the only purpose of this 'non-conventional' mode was
to have 802.1Q acceleration on the ethernet port.
>
>
> We clearly don't have APIs for this, and I don't think it makes sense
> in the Linux space - the bridge and wlan0.N suddenly have tagged
> traffic rather than untagged, and the VLAN tagging is completely hidden
> from the management view.
>
> johannes
^ permalink raw reply
* Re: hung task in mac80211
From: Sebastian Gottschall @ 2017-09-06 15:45 UTC (permalink / raw)
To: Matteo Croce, Johannes Berg; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <CAGnkfhyBMKT-Tf10NxjN40Znuj7OK2pSM3qLUidPSkQqh8gjwQ@mail.gmail.com>
i confirm this patch fixes the issue for me as well
Am 06.09.2017 um 17:04 schrieb Matteo Croce:
> On Wed, Sep 6, 2017 at 2:58 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> On Wed, 2017-09-06 at 13:57 +0200, Matteo Croce wrote:
>>
>>> I have an hung task on vanilla 4.13 kernel which I haven't on 4.12.
>>> The problem is present both on my AP and on my notebook,
>>> so it seems it affects AP and STA mode as well.
>>> The generated messages are:
>>>
>>> INFO: task kworker/u16:6:120 blocked for more than 120 seconds.
>>> Not tainted 4.13.0 #57
>>> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
>>> message.
>>> kworker/u16:6 D 0 120 2 0x00000000
>>> Workqueue: phy0 ieee80211_ba_session_work [mac80211]
>>> Call Trace:
>>> ? __schedule+0x174/0x5b0
>>> ? schedule+0x31/0x80
>>> ? schedule_preempt_disabled+0x9/0x10
>>> ? __mutex_lock.isra.2+0x163/0x480
>>> ? select_task_rq_fair+0xb9f/0xc60
>>> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
>>> ? __ieee80211_start_rx_ba_session+0x135/0x4d0 [mac80211]
>> Yeah - obviously as Stefano found, both take &sta->ampdu_mlme.mtx.
>>
>> Can you try this?
>>
>> diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
>> index 2b36eff5d97e..d8d32776031e 100644
>> --- a/net/mac80211/agg-rx.c
>> +++ b/net/mac80211/agg-rx.c
>> @@ -245,10 +245,10 @@ static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *d
>> ieee80211_tx_skb(sdata, skb);
>> }
>>
>> -void __ieee80211_start_rx_ba_session(struct sta_info *sta,
>> - u8 dialog_token, u16 timeout,
>> - u16 start_seq_num, u16 ba_policy, u16 tid,
>> - u16 buf_size, bool tx, bool auto_seq)
>> +void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
>> + u8 dialog_token, u16 timeout,
>> + u16 start_seq_num, u16 ba_policy, u16 tid,
>> + u16 buf_size, bool tx, bool auto_seq)
>> {
>> struct ieee80211_local *local = sta->sdata->local;
>> struct tid_ampdu_rx *tid_agg_rx;
>> @@ -267,7 +267,7 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
>> ht_dbg(sta->sdata,
>> "STA %pM requests BA session on unsupported tid %d\n",
>> sta->sta.addr, tid);
>> - goto end_no_lock;
>> + goto end;
>> }
>>
>> if (!sta->sta.ht_cap.ht_supported) {
>> @@ -275,14 +275,14 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
>> "STA %pM erroneously requests BA session on tid %d w/o QoS\n",
>> sta->sta.addr, tid);
>> /* send a response anyway, it's an error case if we get here */
>> - goto end_no_lock;
>> + goto end;
>> }
>>
>> if (test_sta_flag(sta, WLAN_STA_BLOCK_BA)) {
>> ht_dbg(sta->sdata,
>> "Suspend in progress - Denying ADDBA request (%pM tid %d)\n",
>> sta->sta.addr, tid);
>> - goto end_no_lock;
>> + goto end;
>> }
>>
>> /* sanity check for incoming parameters:
>> @@ -296,7 +296,7 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
>> ht_dbg_ratelimited(sta->sdata,
>> "AddBA Req with bad params from %pM on tid %u. policy %d, buffer size %d\n",
>> sta->sta.addr, tid, ba_policy, buf_size);
>> - goto end_no_lock;
>> + goto end;
>> }
>> /* determine default buffer size */
>> if (buf_size == 0)
>> @@ -311,7 +311,6 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
>> buf_size, sta->sta.addr);
>>
>> /* examine state machine */
>> - mutex_lock(&sta->ampdu_mlme.mtx);
>>
>> if (test_bit(tid, sta->ampdu_mlme.agg_session_valid)) {
>> if (sta->ampdu_mlme.tid_rx_token[tid] == dialog_token) {
>> @@ -415,15 +414,25 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
>> __clear_bit(tid, sta->ampdu_mlme.unexpected_agg);
>> sta->ampdu_mlme.tid_rx_token[tid] = dialog_token;
>> }
>> - mutex_unlock(&sta->ampdu_mlme.mtx);
>>
>> -end_no_lock:
>> if (tx)
>> ieee80211_send_addba_resp(sta->sdata, sta->sta.addr, tid,
>> dialog_token, status, 1, buf_size,
>> timeout);
>> }
>>
>> +void __ieee80211_start_rx_ba_session(struct sta_info *sta,
>> + u8 dialog_token, u16 timeout,
>> + u16 start_seq_num, u16 ba_policy, u16 tid,
>> + u16 buf_size, bool tx, bool auto_seq)
>> +{
>> + mutex_lock(&sta->ampdu_mlme.mtx);
>> + ___ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
>> + start_seq_num, ba_policy, tid,
>> + buf_size, tx, auto_seq);
>> + mutex_unlock(&sta->ampdu_mlme.mtx);
>> +}
>> +
>> void ieee80211_process_addba_request(struct ieee80211_local *local,
>> struct sta_info *sta,
>> struct ieee80211_mgmt *mgmt,
>> diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
>> index c92df492e898..198b2d3e56fd 100644
>> --- a/net/mac80211/ht.c
>> +++ b/net/mac80211/ht.c
>> @@ -333,9 +333,9 @@ void ieee80211_ba_session_work(struct work_struct *work)
>>
>> if (test_and_clear_bit(tid,
>> sta->ampdu_mlme.tid_rx_manage_offl))
>> - __ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
>> - IEEE80211_MAX_AMPDU_BUF,
>> - false, true);
>> + ___ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
>> + IEEE80211_MAX_AMPDU_BUF,
>> + false, true);
>>
>> if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
>> sta->ampdu_mlme.tid_rx_manage_offl))
>> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
>> index 2197c62a0a6e..9675814f64db 100644
>> --- a/net/mac80211/ieee80211_i.h
>> +++ b/net/mac80211/ieee80211_i.h
>> @@ -1760,6 +1760,10 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
>> u8 dialog_token, u16 timeout,
>> u16 start_seq_num, u16 ba_policy, u16 tid,
>> u16 buf_size, bool tx, bool auto_seq);
>> +void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
>> + u8 dialog_token, u16 timeout,
>> + u16 start_seq_num, u16 ba_policy, u16 tid,
>> + u16 buf_size, bool tx, bool auto_seq);
>> void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
>> enum ieee80211_agg_stop_reason reason);
>> void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,
>>
>> johannes
> I confirm that this patch fixes the hang too.
> I'm curious to see if there are noticeable performance differences
> between the two solutions.
>
> Cheers,
--
Mit freundlichen Grüssen / Regards
Sebastian Gottschall / CTO
NewMedia-NET GmbH - DD-WRT
Firmensitz: Berliner Ring 101, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565
^ permalink raw reply
* Re: [PATCH net-next] xdp: implement xdp_redirect_map for generic XDP
From: Andy Gospodarek @ 2017-09-06 15:44 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev@vger.kernel.org, David S. Miller, John Fastabend
In-Reply-To: <150471158528.3727.12324542627400287360.stgit@firesoul>
On Wed, Sep 6, 2017 at 11:26 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> Using bpf_redirect_map is allowed for generic XDP programs, but the
> appropriate map lookup was never performed in xdp_do_generic_redirect().
>
> Instead the map-index is directly used as the ifindex. For the
> xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
> sending on ifindex 0 which isn't valid, resulting in getting SKB
> packets dropped. Thus, the reported performance numbers are wrong in
> commit 24251c264798 ("samples/bpf: add option for native and skb mode
> for redirect apps") for the 'xdp_redirect_map -S' case.
>
> It might seem innocent this was lacking, but it can actually crash the
> kernel. The potential crash is caused by not consuming redirect_info->map.
> The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map
> pointer, which will survive even after unloading the xdp bpf_prog and
> deallocating the devmap data-structure. This leaves a dead map
> pointer around. The kernel will crash when loading the xdp_redirect
> sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)
> and returns XDP_REDIRECT, which will cause it to dereference the map
> pointer.
Nice catch!
Since 'net-next' is closed and this is a bugfix it seems like this is
a good candidate for 'net' right?
>
> Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
> Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Andy Gospodarek <andy@greyhouse.net>
> ---
> net/core/filter.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 5912c738a7b2..6a4745bf2c9f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2562,6 +2562,32 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
> }
> EXPORT_SYMBOL_GPL(xdp_do_redirect);
>
> +static int xdp_do_generic_redirect_map(struct net_device *dev,
> + struct sk_buff *skb,
> + struct bpf_prog *xdp_prog)
> +{
> + struct redirect_info *ri = this_cpu_ptr(&redirect_info);
> + struct bpf_map *map = ri->map;
> + u32 index = ri->ifindex;
> + struct net_device *fwd;
> + int err;
> +
> + ri->ifindex = 0;
> + ri->map = NULL;
> +
> + fwd = __dev_map_lookup_elem(map, index);
> + if (!fwd) {
> + err = -EINVAL;
> + goto err;
> + }
> + skb->dev = fwd;
> + _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
> + return 0;
> +err:
> + _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
> + return err;
> +}
> +
> int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> struct bpf_prog *xdp_prog)
> {
> @@ -2571,6 +2597,9 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> unsigned int len;
> int err = 0;
>
> + if (ri->map)
> + return xdp_do_generic_redirect_map(dev, skb, xdp_prog);
> +
> fwd = dev_get_by_index_rcu(dev_net(dev), index);
> ri->ifindex = 0;
> if (unlikely(!fwd)) {
>
^ permalink raw reply
* Re: [PATCH v2 rfc 4/8] net: dsa: slave: Handle switchdev host mdb add/del
From: Vivien Didelot @ 2017-09-06 15:37 UTC (permalink / raw)
To: Andrew Lunn, netdev; +Cc: jiri, nikolay, Florian Fainelli, Andrew Lunn
In-Reply-To: <1504654510-31004-5-git-send-email-andrew@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
> 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;
If SWITCHDEV_OBJ_ID_HOST_MDB is really necessary,
case SWITCHDEV_OBJ_ID_HOST_MDB:
err = dsa_port_mdb_add(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj), trans);
break;
should be enough. DSA_NOTIFIER_HOST_MDB_* are not necessary.
Thanks,
Vivien
^ permalink raw reply
* Fw: [Bug 196839] New: use_time of IPsec policy is updated even when receiving error packets.
From: Stephen Hemminger @ 2017-09-06 15:38 UTC (permalink / raw)
To: netdev
Begin forwarded message:
Date: Wed, 06 Sep 2017 10:18:33 +0000
From: bugzilla-daemon@bugzilla.kernel.org
To: stephen@networkplumber.org
Subject: [Bug 196839] New: use_time of IPsec policy is updated even when receiving error packets.
https://bugzilla.kernel.org/show_bug.cgi?id=196839
Bug ID: 196839
Summary: use_time of IPsec policy is updated even when
receiving error packets.
Product: Networking
Version: 2.5
Kernel Version: 4.8.0
Hardware: Intel
OS: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: Other
Assignee: stephen@networkplumber.org
Reporter: cchenme@gmail.com
Regression: No
Normally the use_time of policy in SPD is updated if the policy is matched by
incoming or outgoing IP packets. For protect policy, it is updated if ESP/AH
packets are sent or received.
The use_time of SPD_IN policy used by IKE implementation like strongSwan to
check whether there is inbound traffic, thus determine whether it is necessary
to send DPD(dead peer detection, rfc3706) request to check liveness of IPsec
peer.
In case an unprotected packet is received but matches the IPsec SPD IN protect
policy, the packet will be discarded and the error counter XfrmInTmplMismatch
in /proc/net/xfrm_stat is incremented.
But in such error/malicious case, the use_time of SPD IN policy is also
updated. This cause strongSwan to mistakenly regard that the policy is in use
and not to trigger DPD request even when it should.
In short, this is a security hole in kernel and could lead to DoS attack on
IPsec gateway running on Linux.
--
You are receiving this mail because:
You are the assignee for the bug.
^ permalink raw reply
* [PATCH net] rds: Fix incorrect statistics counting
From: Håkon Bugge @ 2017-09-06 15:29 UTC (permalink / raw)
To: Santosh Shilimkar, David S . Miller
Cc: netdev, linux-rdma, rds-devel, linux-kernel, knut.omang
In rds_send_xmit() there is logic to batch the sends. However, if
another thread has acquired the lock, it is considered a race and we
yield. The code incrementing the s_send_lock_queue_raced statistics
counter did not count this event correctly.
This commit removes a small race in determining the race and
increments the statistics counter correctly.
Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
Reviewed-by: Knut Omang <knut.omang@oracle.com>
---
net/rds/send.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/net/rds/send.c b/net/rds/send.c
index 058a407..ecfe0b5 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -101,6 +101,11 @@ void rds_send_path_reset(struct rds_conn_path *cp)
}
EXPORT_SYMBOL_GPL(rds_send_path_reset);
+static bool someone_in_xmit(struct rds_conn_path *cp)
+{
+ return test_bit(RDS_IN_XMIT, &cp->cp_flags);
+}
+
static int acquire_in_xmit(struct rds_conn_path *cp)
{
return test_and_set_bit(RDS_IN_XMIT, &cp->cp_flags) == 0;
@@ -428,14 +433,19 @@ int rds_send_xmit(struct rds_conn_path *cp)
* some work and we will skip our goto
*/
if (ret == 0) {
+ bool raced;
+
smp_mb();
+ raced = someone_in_xmit(cp) ||
+ send_gen != READ_ONCE(cp->cp_send_gen);
+
if ((test_bit(0, &conn->c_map_queued) ||
- !list_empty(&cp->cp_send_queue)) &&
- send_gen == READ_ONCE(cp->cp_send_gen)) {
- rds_stats_inc(s_send_lock_queue_raced);
+ !list_empty(&cp->cp_send_queue)) && !raced) {
if (batch_count < send_batch_count)
goto restart;
queue_delayed_work(rds_wq, &cp->cp_send_w, 1);
+ } else if (raced) {
+ rds_stats_inc(s_send_lock_queue_raced);
}
}
out:
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v2 rfc 0/8] IGMP snooping for local traffic
From: Vivien Didelot @ 2017-09-06 15:25 UTC (permalink / raw)
To: Andrew Lunn, netdev; +Cc: jiri, nikolay, Florian Fainelli, Andrew Lunn
In-Reply-To: <1504654510-31004-1-git-send-email-andrew@lunn.ch>
Hi Andrew, Nikolay,
Andrew Lunn <andrew@lunn.ch> writes:
> 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...
I'm not sure if you already explained that, if so, sorry in advance.
I don't understand why SWITCHDEV_OBJ_ID_PORT_MDB cannot be used. Isn't
setting the obj->orig_dev to the bridge device itself enough to
distinguish br0 from a switch port?
This way, the only change necessary in net/dsa/slave.c is something
like (abbreviated):
static int dsa_slave_port_obj_add(struct net_device *dev,
const struct switchdev_obj *obj,
struct switchdev_trans *trans)
{
struct dsa_slave_priv *p = netdev_priv(dev);
struct dsa_port *port = p->dp;
/* Is the target port the bridge device itself? */
if (obj->orig_dev == port->br)
port = port->cpu_dp;
return dsa_port_mdb_add(port, obj, trans);
}
The main problem is that we will soon want support for multiple CPU
ports. This means that each DSA slave will have its dedicated CPU port,
instead of having only one for the whole switch tree.
So adding the MDB entry to all CPU ports as you did in a patch 5/8 in
dsa_switch_host_mdb_*() is not correct because you can have CPU port
sw0p0 being a member of br0 and CPU port sw0p10 being a member of br1.
So is it correct to simply notify SWITCHDEV_OBJ_ID_PORT_MDB with
orig_dev = br->dev so that its members get recursively notified?
Even if SWITCHDEV_OBJ_ID_HOST_MDB is necessary, we need to handle it the
way I described, otherwise we don't have a correct mapping between a
slave port and its CPU port that we need to configure.
Thanks,
Vivien
^ 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