* Re: [PATCH bpf-next] bpf: add skb->queue_mapping write access from tc clsact
From: Daniel Borkmann @ 2019-02-19 16:18 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netdev, Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <20190219155259.677d195c@carbon>
On 02/19/2019 03:52 PM, Jesper Dangaard Brouer wrote:
> On Tue, 19 Feb 2019 12:46:57 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>> On 02/19/2019 11:24 AM, Jesper Dangaard Brouer wrote:
>>> The skb->queue_mapping already have read access, via __sk_buff->queue_mapping.
>>>
>>> This patch allow BPF tc qdisc clsact write access to the queue_mapping via
>>> tc_cls_act_is_valid_access.
>>>
>>> It is already possible to change this via TC filter action skbedit
>>> tc-skbedit(8). Due to the lack of TC examples, lets show one:
>>>
>>> # tc qdisc add dev ixgbe1 handle ffff: ingress
>>> # tc filter add dev ixgbe1 parent ffff: matchall action skbedit queue_mapping 5
>>> # tc filter list dev ixgbe1 parent ffff:
>>
>> Using handles was in the old days, if we add examples, then lets do
>> something more user friendly ;)
>>
>> # tc qdisc add dev ixgbe1 clsact
>> # tc filter replace dev ixgbe1 ingress matchall action skbedit queue_mapping 5
>> # tc filter list dev ixgbe1 ingress
>>
>>> The most common mistake is that XPS (Transmit Packet Steering) takes
>>> precedence over setting skb->queue_mapping. XPS is configured per DEVICE
>>> via /sys/class/net/DEVICE/queues/tx-*/xps_cpus via a CPU hex mask. To
>>> disable set mask=00.
>>>
>>> The purpose of changing skb->queue_mapping is to influence the selection of
>>> the net_device "txq" (struct netdev_queue), which influence selection of
>>> the qdisc "root_lock" (via txq->qdisc->q.lock) and txq->_xmit_lock. When
>>> using the MQ qdisc the txq->qdisc points to different qdiscs and associated
>>> locks, and HARD_TX_LOCK (txq->_xmit_lock), allowing for CPU scalability.
>>>
>>> Due to lack of TC examples, lets show howto attach clsact BPF programs:
>>>
>>> # tc qdisc add dev ixgbe2 clsact
>>> # tc filter replace dev ixgbe2 egress bpf da obj XXX_kern.o sec tc_qmap2cpu
>>> # tc filter list dev ixgbe2 egress
>>>
>>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>> ---
>>> net/core/filter.c | 14 +++++++++++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 353735575204..d05ae8d05397 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -6238,6 +6238,7 @@ static bool tc_cls_act_is_valid_access(int off, int size,
>>> case bpf_ctx_range(struct __sk_buff, tc_classid):
>>> case bpf_ctx_range_till(struct __sk_buff, cb[0], cb[4]):
>>> case bpf_ctx_range(struct __sk_buff, tstamp):
>>> + case bpf_ctx_range(struct __sk_buff, queue_mapping):
>>> break;
>>> default:
>>> return false;
>>> @@ -6642,9 +6643,16 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
>>> break;
>>>
>>> case offsetof(struct __sk_buff, queue_mapping):
>>> - *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
>>> - bpf_target_off(struct sk_buff, queue_mapping, 2,
>>> - target_size));
>>> + if (type == BPF_WRITE)
>>> + *insn++ = BPF_STX_MEM(BPF_H, si->dst_reg, si->src_reg,
>>> + bpf_target_off(struct sk_buff,
>>> + queue_mapping,
>>> + 2, target_size));
>>> + else
>>> + *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
>>> + bpf_target_off(struct sk_buff,
>>> + queue_mapping,
>>> + 2, target_size));
>>
>> One thing we should avoid would be to allow user to write NO_QUEUE_MAPPING
>> into skb->queue_mapping so we don't hit the warn in sk_tx_queue_set(), I'd
>> add this into the ctx rewrite here.
>
> Makes sense. I would really appreciate if you could help me out writing
> the needed BPF instructions, as I'm not an expert here.
Untested / uncompiled, but should be:
case offsetof(struct __sk_buff, queue_mapping):
if (type == BPF_WRITE) {
*insn++ = BPF_JMP_IMM(BPF_JGE, si->src_reg, NO_QUEUE_MAPPING, 1);
*insn++ = BPF_STX_MEM(BPF_H, si->dst_reg, si->src_reg,
bpf_target_off(struct sk_buff, queue_mapping, 2,
target_size));
} else {
*insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
bpf_target_off(struct sk_buff, queue_mapping, 2,
target_size));
}
break;
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Russell King - ARM Linux admin @ 2019-02-19 16:24 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219111612.GF27578@t480s.localdomain>
On Tue, Feb 19, 2019 at 11:16:12AM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> > +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
> > + unsigned long flags)
> > +{
> > + struct mv88e6xxx_chip *chip = ds->priv;
> > + bool unicast, multicast;
> > + int ret = -EOPNOTSUPP;
> > +
> > + unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
> > + flags & BR_FLOOD;
> > + multicast = flags & BR_MCAST_FLOOD;
> > +
> > + mutex_lock(&chip->reg_lock);
> > + if (chip->info->ops->port_set_egress_floods)
> > + ret = chip->info->ops->port_set_egress_floods(chip, port,
> > + unicast,
> > + multicast);
> > + mutex_unlock(&chip->reg_lock);
> > +
> > + return ret;
> > +}
> > +
> > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > +{
> > + struct mv88e6xxx_chip *chip = ds->priv;
> > + unsigned long support = 0;
> > +
> > + if (chip->info->ops->port_set_egress_floods)
> > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > +
> > + return support;
> > +}
>
> I think that it isn't necessary to propagate the notion of bridge flags down
> to the DSA drivers. It might be just enough to add:
>
> port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
>
> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
There are two other flags that I haven't covered which the bridge code
expects to be offloaded, and those are the broadcast flood flag and
the learning flag.
I know that the Marvell switches don't have a bit to control the
broadcast flooding, that appears to be controlled via a static entry
in the ATU which would have to be modified as the broadcast flood flag
is manipulated. I don't know how that is handled in other bridges.
Do we want to include the broadcast flood in the above prototype?
If we go for this, how do we detect which options a switch supports?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH][next] vhost: only return early if ret indicates an error or no iovecs have been processed
From: Michael S. Tsirkin @ 2019-02-19 16:35 UTC (permalink / raw)
To: Colin King
Cc: Jason Wang, kvm, virtualization, netdev, kernel-janitors,
linux-kernel
In-Reply-To: <20190219135713.10426-1-colin.king@canonical.com>
On Tue, Feb 19, 2019 at 01:57:13PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently the loop that calls log_write_hva on each iovec is never
> executed because of an incorrect error check on the return from the
> call to translate_desc. The check should be checking for a -ve error
> return and because it makes no sense to iterate over zero items, the
> checks should also check for zero too.
>
> Detected by CoverityScan, CID#1476969 ("Logically dead code")
>
> Fixes: cc5e71075947 ("vhost: log dirty page correctly")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Jason posted a similar patch recently.
Are you happy with that one?
> ---
> drivers/vhost/vhost.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 24a129fcdd61..a9a1709a859a 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1788,7 +1788,7 @@ static int log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
>
> ret = translate_desc(vq, (uintptr_t)vq->used + used_offset,
> len, iov, 64, VHOST_ACCESS_WO);
> - if (ret)
> + if (ret <= 0)
> return ret;
>
> for (i = 0; i < ret; i++) {
> --
> 2.20.1
^ permalink raw reply
* Re: L2TPv3 offset
From: James Chapman @ 2019-02-19 16:36 UTC (permalink / raw)
To: t.martitz; +Cc: davem, netdev
In-Reply-To: <OFF1459E30.8430BF87-ONC12583A6.0047B081-C12583A6.004841EE@avm.de>
On 19/02/2019 13:09, t.martitz@avm.de wrote:
>
> Hello,
>
> thanks for your quick response.
>
> "James Chapman" <jchapman@katalix.com> schrieb am 19.02.2019 13:40:10:
>
> > Von: "James Chapman" <jchapman@katalix.com>
> > An: t.martitz@avm.de
> > Kopie: davem@davemloft.net, "netdev" <netdev@vger.kernel.org>
> > Datum: 19.02.2019 13:40
> > Betreff: Re: L2TPv3 offset
> >
> > On 19/02/2019 09:17, t.martitz@avm.de wrote:
> > >
> > > Hello,
> > >
> > > I saw that you removed the offset option from l2tp sessions in Linux
> > > 4.16 (commit 900631ee6a2651dc4fbaecb8ef9fa5f1e3378853 l2tp: remove
> > > configurable payload offset). Since we need something like that I'm
> > > reaching out to you.
> > >
> > Adding netdev.
> > >
> > >
> > > Our use case is pseudo-wire over l2tp, so we encapsulate ethernet
> > > within l2tpv3. This has lots of benifits for us, but one remarkable
> > > drawback is that the receiver, who's removing the encapsulation, will
> > > see a misaligned inner IP header.
> > >
> > > So we planned to use the offset to correct that. But I saw that the
> > > offset was removed not too long ago. Now I see that there is still
> > > "l2specific_len" / L2TP_ATTR_L2SPEC_LEN that we can use for pretty
> > > much the same purpose but I get the feeling that this should only be 0
> > > or 4 while we would need 2 or 6. I get this feeling because "ip l2tp"
> > > of the iproute2 package does not allow setting this at all and
> > > hardcodes 0 or 4 based on the type.
> > >
> > In L2TPv3, any payload offset would need to be implemented by defining a
> > new L2SpecificSublayer type since the L2TPv3 header has no offset field.
> > There are two standard L2TPv3 sublayers supported - None and Default.
> > These are fixed size (0 and 4 bytes respectively), hence the kernel now
> > ignores L2TP_ATTR_L2SPEC_LEN since the length can be derived from
> > L2TP_ATTR_L2SPEC_TYPE. For reference, the set of defined types are
> > listed at
> > https://www.iana.org/assignments/l2tp-parameters/l2tp-
> > parameters.xhtml#l2tp-parameters-37.
> > No L2SpecificSublayer type was ever defined for L2TPv3 to allow a
> > configurable payload offset in ethernet pseudowires.
>
>
> So are we the only ones to encapsulate Ethernet within L2TPv3? It's
> hard to believe.
>
No of course not.
>
>
> Because everytime a LCCE decapsulates such traffic it'll suffer from
> unaligned access to the inner IP header (likewise for the outer IP
> header when encapsulating). It's a fundamental assumption that the IP
> header is word-aligned in Linux so I'm surprised this isn't solved
> already. And now the only way "fix" without patching the kernel is
> being removed.
>
It was removed to prevent the possibility of sending non-compliant
L2TPv3 packets into the network. L2TPv3 allows for new
L2SpecificSublayer header types to be defined. It's a shame that one
hasn't been added to allow for this.
Are you using a CPU that can't do unaligned word accesses?
>
> >
> > So my question is what should we do know? Being based on removed
> > functionality is kind of bad, but we must fix the misaligned inner ip
> > header.
> > We control the sender and receiver so we could apply all kinds of
> > hacks, but we would rather find a solution compliant with the Internet
> > community.
> >
> If you really need to insert padding in transmitted L2TPv3 packets
> between the L2TPv3 header and its payload, one option is to define a new
> L2SpecificHeaderType and patch the kernel to accept it.
>
> If you control both the sender and receiver, is using FOU an option?
>
What does FOU refer to?
Foo-Over-UDP. See ip-fou(8).
^ permalink raw reply
* Re: [PATCH][next] vhost: only return early if ret indicates an error or no iovecs have been processed
From: Colin Ian King @ 2019-02-19 16:40 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, kvm, virtualization, netdev, kernel-janitors,
linux-kernel
In-Reply-To: <20190219113510-mutt-send-email-mst@kernel.org>
On 19/02/2019 16:35, Michael S. Tsirkin wrote:
> On Tue, Feb 19, 2019 at 01:57:13PM +0000, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Currently the loop that calls log_write_hva on each iovec is never
>> executed because of an incorrect error check on the return from the
>> call to translate_desc. The check should be checking for a -ve error
>> return and because it makes no sense to iterate over zero items, the
>> checks should also check for zero too.
>>
>> Detected by CoverityScan, CID#1476969 ("Logically dead code")
>>
>> Fixes: cc5e71075947 ("vhost: log dirty page correctly")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>
> Jason posted a similar patch recently.
>
> Are you happy with that one?
Sure, ignore mine, I was late to the party.
>
>> ---
>> drivers/vhost/vhost.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 24a129fcdd61..a9a1709a859a 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -1788,7 +1788,7 @@ static int log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
>>
>> ret = translate_desc(vq, (uintptr_t)vq->used + used_offset,
>> len, iov, 64, VHOST_ACCESS_WO);
>> - if (ret)
>> + if (ret <= 0)
>> return ret;
>>
>> for (i = 0; i < ret; i++) {
>> --
>> 2.20.1
^ permalink raw reply
* [PATCH net 0/2] report erspan version field just for erspan tunnels
From: Lorenzo Bianconi @ 2019-02-19 16:42 UTC (permalink / raw)
To: davem; +Cc: netdev, u9012063
Do not report erspan_version to userpsace for non erspan tunnels.
Report IFLA_GRE_ERSPAN_INDEX only for erspan version 1 in
ip6gre_fill_info
Lorenzo Bianconi (2):
net: ip_gre: do not report erspan_ver for gre or gretap
net: ip6_gre: do not report erspan_ver for ip6gre or ip6gretap
net/ipv4/ip_gre.c | 33 +++++++++++++++++----------------
net/ipv6/ip6_gre.c | 36 ++++++++++++++++++------------------
2 files changed, 35 insertions(+), 34 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH net 1/2] net: ip_gre: do not report erspan_ver for gre or gretap
From: Lorenzo Bianconi @ 2019-02-19 16:42 UTC (permalink / raw)
To: davem; +Cc: netdev, u9012063
In-Reply-To: <cover.1550594081.git.lorenzo.bianconi@redhat.com>
Report erspan version field to userspace in ipgre_fill_info just for
erspan tunnels. The issue can be triggered with the following reproducer:
$ip link add name gre1 type gre local 192.168.0.1 remote 192.168.1.1
$ip link set dev gre1 up
$ip -d link sh gre1
13: gre1@NONE: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1476 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
link/gre 192.168.0.1 peer 192.168.1.1 promiscuity 0 minmtu 0 maxmtu 0
gre remote 192.168.1.1 local 192.168.0.1 ttl inherit erspan_ver 0 addrgenmode eui64 numtxqueues 1 numrxqueues 1
Fixes: f551c91de262 ("net: erspan: introduce erspan v2 for ip_gre")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
net/ipv4/ip_gre.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 3978f807fa8b..6ae89f2b541b 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1457,9 +1457,23 @@ static int ipgre_fill_info(struct sk_buff *skb, const struct net_device *dev)
struct ip_tunnel_parm *p = &t->parms;
__be16 o_flags = p->o_flags;
- if ((t->erspan_ver == 1 || t->erspan_ver == 2) &&
- !t->collect_md)
- o_flags |= TUNNEL_KEY;
+ if (t->erspan_ver == 1 || t->erspan_ver == 2) {
+ if (!t->collect_md)
+ o_flags |= TUNNEL_KEY;
+
+ if (nla_put_u8(skb, IFLA_GRE_ERSPAN_VER, t->erspan_ver))
+ goto nla_put_failure;
+
+ if (t->erspan_ver == 1) {
+ if (nla_put_u32(skb, IFLA_GRE_ERSPAN_INDEX, t->index))
+ goto nla_put_failure;
+ } else {
+ if (nla_put_u8(skb, IFLA_GRE_ERSPAN_DIR, t->dir))
+ goto nla_put_failure;
+ if (nla_put_u16(skb, IFLA_GRE_ERSPAN_HWID, t->hwid))
+ goto nla_put_failure;
+ }
+ }
if (nla_put_u32(skb, IFLA_GRE_LINK, p->link) ||
nla_put_be16(skb, IFLA_GRE_IFLAGS,
@@ -1495,19 +1509,6 @@ static int ipgre_fill_info(struct sk_buff *skb, const struct net_device *dev)
goto nla_put_failure;
}
- if (nla_put_u8(skb, IFLA_GRE_ERSPAN_VER, t->erspan_ver))
- goto nla_put_failure;
-
- if (t->erspan_ver == 1) {
- if (nla_put_u32(skb, IFLA_GRE_ERSPAN_INDEX, t->index))
- goto nla_put_failure;
- } else if (t->erspan_ver == 2) {
- if (nla_put_u8(skb, IFLA_GRE_ERSPAN_DIR, t->dir))
- goto nla_put_failure;
- if (nla_put_u16(skb, IFLA_GRE_ERSPAN_HWID, t->hwid))
- goto nla_put_failure;
- }
-
return 0;
nla_put_failure:
--
2.20.1
^ permalink raw reply related
* [PATCH net 2/2] net: ip6_gre: do not report erspan_ver for ip6gre or ip6gretap
From: Lorenzo Bianconi @ 2019-02-19 16:42 UTC (permalink / raw)
To: davem; +Cc: netdev, u9012063
In-Reply-To: <cover.1550594081.git.lorenzo.bianconi@redhat.com>
Report erspan version field to userspace in ip6gre_fill_info just for
erspan_v6 tunnels. Moreover report IFLA_GRE_ERSPAN_INDEX only for
erspan version 1.
The issue can be triggered with the following reproducer:
$ip link add name gre6 type ip6gre local 2001::1 remote 2002::2
$ip link set gre6 up
$ip -d link sh gre6
14: grep6@NONE: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1448 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
link/gre6 2001::1 peer 2002::2 promiscuity 0 minmtu 0 maxmtu 0
ip6gre remote 2002::2 local 2001::1 hoplimit 64 encaplimit 4 tclass 0x00 flowlabel 0x00000 erspan_index 0 erspan_ver 0 addrgenmode eui64
Fixes: 94d7d8f29287 ("ip6_gre: add erspan v2 support")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
net/ipv6/ip6_gre.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 43890898b0b5..0fdd0109d131 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -2104,9 +2104,23 @@ static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
struct __ip6_tnl_parm *p = &t->parms;
__be16 o_flags = p->o_flags;
- if ((p->erspan_ver == 1 || p->erspan_ver == 2) &&
- !p->collect_md)
- o_flags |= TUNNEL_KEY;
+ if (p->erspan_ver == 1 || p->erspan_ver == 2) {
+ if (!p->collect_md)
+ o_flags |= TUNNEL_KEY;
+
+ if (nla_put_u8(skb, IFLA_GRE_ERSPAN_VER, p->erspan_ver))
+ goto nla_put_failure;
+
+ if (p->erspan_ver == 1) {
+ if (nla_put_u32(skb, IFLA_GRE_ERSPAN_INDEX, p->index))
+ goto nla_put_failure;
+ } else {
+ if (nla_put_u8(skb, IFLA_GRE_ERSPAN_DIR, p->dir))
+ goto nla_put_failure;
+ if (nla_put_u16(skb, IFLA_GRE_ERSPAN_HWID, p->hwid))
+ goto nla_put_failure;
+ }
+ }
if (nla_put_u32(skb, IFLA_GRE_LINK, p->link) ||
nla_put_be16(skb, IFLA_GRE_IFLAGS,
@@ -2121,8 +2135,7 @@ static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
nla_put_u8(skb, IFLA_GRE_ENCAP_LIMIT, p->encap_limit) ||
nla_put_be32(skb, IFLA_GRE_FLOWINFO, p->flowinfo) ||
nla_put_u32(skb, IFLA_GRE_FLAGS, p->flags) ||
- nla_put_u32(skb, IFLA_GRE_FWMARK, p->fwmark) ||
- nla_put_u32(skb, IFLA_GRE_ERSPAN_INDEX, p->index))
+ nla_put_u32(skb, IFLA_GRE_FWMARK, p->fwmark))
goto nla_put_failure;
if (nla_put_u16(skb, IFLA_GRE_ENCAP_TYPE,
@@ -2140,19 +2153,6 @@ static int ip6gre_fill_info(struct sk_buff *skb, const struct net_device *dev)
goto nla_put_failure;
}
- if (nla_put_u8(skb, IFLA_GRE_ERSPAN_VER, p->erspan_ver))
- goto nla_put_failure;
-
- if (p->erspan_ver == 1) {
- if (nla_put_u32(skb, IFLA_GRE_ERSPAN_INDEX, p->index))
- goto nla_put_failure;
- } else if (p->erspan_ver == 2) {
- if (nla_put_u8(skb, IFLA_GRE_ERSPAN_DIR, p->dir))
- goto nla_put_failure;
- if (nla_put_u16(skb, IFLA_GRE_ERSPAN_HWID, p->hwid))
- goto nla_put_failure;
- }
-
return 0;
nla_put_failure:
--
2.20.1
^ permalink raw reply related
* Re: [PATCH RFC 2/5] net/sched: prepare TC actions to properly validate the control action
From: Davide Caratti @ 2019-02-19 16:50 UTC (permalink / raw)
To: Vlad Buslov
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Paolo Abeni, netdev@vger.kernel.org
In-Reply-To: <vbfbm39qbsw.fsf@mellanox.com>
hi Vlad,
On Mon, 2019-02-18 at 16:52 +0000, Vlad Buslov wrote:
> Hi Davide,
>
> I really like the idea of putting all action validation code in single
> place, instead of spreading it between act API and specific actions init
> code.
sure, the previous validation code was in tcf_action_add_1(), and I
attempted to fix the problems (well, it's the same problem showing up in
3 different ways) without adding the same code everywhere.
Sadly, we need to handle correctly the situations where
tcf_action_check_ctrlact() returns an error, and I'm seeing that we
can't fix every action with the same exact code everywhere (to avoid
leaking IDRs, or memory, or both).
> See my comment regarding minor problem with using
> tcf_action_set_ctrlact() on current net-next below.
> > +void tcf_action_set_ctrlact(struct tc_action *p, int action,
> > + struct tcf_chain *goto_chain)
> > +{
> > + struct tcf_chain *old;
> > +
> > + old = xchg(&p->goto_chain, goto_chain);
> > + if (old)
> > + tcf_chain_put_by_act(old);
>
> This is blocking in current net-next because tcf_chain_put_by_act()
> obtains block->lock mutex, so you can't call it while holding tcf_lock
> spinlock like you do in following patches. Its not a big problem but
> I guess you will have to just inline this code into all init handlers
> instead of tcf_action_set_ctrlact() function.
Argh! you are right. Thanks a lot for spotting this.
Ok, let's kill tcf_action_set_ctrlact() and swap 'newchain' with
a->goto_chain in each action init(), with a->tcfa_lock taken. I will
then call tcf_chain_put_by_act() in the init() handler, after the
spinlock is released.
> BTW is atomic xchg strictly necessary if you hold tcf_lock while
> re-assigning goto_chain pointer?
No, it's not necessary. The new value of goto_chain is used only when the
new value of tcfa_action is updated: so, if we just do the assignment of
goto_chain and tcfa_action together with the spinlock taken, at least we
are improving the
current code.
Thanks!
--
davide
^ permalink raw reply
* Re: [PATCH RFC 0/5] net/sched: validate the control action with all the other parameters
From: Davide Caratti @ 2019-02-19 16:51 UTC (permalink / raw)
To: Cong Wang
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Vlad Buslov,
Paolo Abeni, Linux Kernel Network Developers
In-Reply-To: <CAM_iQpX5ogcHsX+9JFxj-C8XMQmHxVM+fV+rkvv9mBK2yL2zNQ@mail.gmail.com>
On Mon, 2019-02-18 at 22:42 -0800, Cong Wang wrote:
> On Fri, Feb 15, 2019 at 3:06 PM Davide Caratti <dcaratti@redhat.com> wrote:
> > currently, the kernel checks for bad values of the control action in
> > tcf_action_init_1(), after a successful call to the action's init()
> > function. This causes three bad behaviors:
>
> Yeah, I have been complaining about this for a long time,
> although slightly differently. The problem here is the lack of
> "copy" in RCU mechanism, which makes it nearly impossible
> to rollback to the previous state of an action on failure path
> of an update, which also makes RCU readers reading a partially
> updated action too.
thanks for looking at this code.
by the way, I see that act_mirred has an error path after the
assignment of tcfm_eaction and tcfa_action, and this is again causing
a fail in the 'replace with bad action' tests ('half write', issue #1).
Since it's the same problem, I will fix this in the same patch (moving the
assignment after the 'if' test on the value of parm->ifindex.
> Before I fix the "copy" part, your fixes make sense to me. There
> might be some other way to expose the action-specific tcfa_action
> opcode, but it might not be better than yours.
>
> BTW, please fold these bad behaviors into each appropriate
> patch, and keep the cover letter as an overview of the whole
> patchset rather than showing any details.
>
> [...]
Ok, and I plan to add a selftest for each action - so that it's possible
to verify functionality (at least problem #1) before and after each
ommit.
> > all these three problems can be fixed if we validate the control action
> > in the init() function, in the same way as we are already doing for all
> > the other parameters.
> >
> > - patch 1 is a temporary fix for problem 2), but it's reverted at the
> > end of the series
>
> Please drop patch 1, it is very unlikely only patch 1 will be backported,
> I think the whole patchset should be, therefore we have no reason
> to carry a temporary fix here.
sure, I will do that.
thanks!
--
davide
^ permalink raw reply
* Re: [PATCH 0/2] ARM: dts: am335x-evm/evmsk: Fix PHY mode for ethernet
From: Tony Lindgren @ 2019-02-19 16:52 UTC (permalink / raw)
To: Peter Ujfalusi
Cc: bcousson, linux-omap, devicetree, linux-arm-kernel, nsekhar,
grygorii.strashko, vkoul, netdev, f.fainelli, marc.w.gonzalez,
niklas.cassel
In-Reply-To: <c35030fb-9107-1919-5c18-1a39ace8c036@ti.com>
* Peter Ujfalusi <peter.ujfalusi@ti.com> [190219 08:02]:
> On 18/02/2019 18.26, Tony Lindgren wrote:
> > OK so sounds like these are OK to wait for v5.1 merge window
> > then as the dts changes alone won't fix anything?
>
> I think it would be better to send these to 5.0 to avoid the regression
> siting in linux-next.
>
> The patches are actually doing to the correct thing (we need rgmii-id,
> not rmgii-txid for these boards).
> It was just a matter of luck that it worked with rgmii-txid as the
> driver was broken and did not disabled the rxid when it should have.
OK applying into omap-for-v5.0/fixes-v2. I'll send a pull
request on Thursday most likely then, let's see if it makes
it for v5.0.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH bpf-next v2] bpf: bpftool, fix documentation for attach types
From: Daniel Borkmann @ 2019-02-19 16:24 UTC (permalink / raw)
To: Alban Crequy, ast, quentin.monnet
Cc: netdev, linux-kernel, john.fastabend, alban, iago
In-Reply-To: <20190219141332.23103-1-alban@kinvolk.io>
On 02/19/2019 03:13 PM, Alban Crequy wrote:
> From: Alban Crequy <alban@kinvolk.io>
>
> bpftool has support for attach types "stream_verdict" and
> "stream_parser" but the documentation was referring to them as
> "skb_verdict" and "skb_parse". The inconsistency comes from commit
> b7d3826c2ed6 ("bpf: bpftool, add support for attaching programs to
> maps").
>
> This patch changes the documentation to match the implementation:
> - "bpftool prog help"
> - man pages
> - bash completion
>
> Signed-off-by: Alban Crequy <alban@kinvolk.io>
Applied, thanks Alban!
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 17:00 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219162435.f5zl5harbarwy6bj@shell.armlinux.org.uk>
Hi Russell,
On Tue, 19 Feb 2019 16:24:35 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > +{
> > > + struct mv88e6xxx_chip *chip = ds->priv;
> > > + unsigned long support = 0;
> > > +
> > > + if (chip->info->ops->port_set_egress_floods)
> > > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > +
> > > + return support;
> > > +}
> >
> > I think that it isn't necessary to propagate the notion of bridge flags down
> > to the DSA drivers. It might be just enough to add:
> >
> > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> >
> > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
>
> There are two other flags that I haven't covered which the bridge code
> expects to be offloaded, and those are the broadcast flood flag and
> the learning flag.
I see. What does the bridge code do if these flags are set? Does it expect
the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
program this entry into the bridged ports?
In the latter case we have almost nothing to do. In the former case, we can
make the core call dsa_port_mdb_add on setup and when a VLAN is added.
mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
If tomorrow there's a switch capable of simply toggling a bit to do that,
we can add a new ops and skip the port_mdb_add call in the core.
> I know that the Marvell switches don't have a bit to control the
> broadcast flooding, that appears to be controlled via a static entry
> in the ATU which would have to be modified as the broadcast flood flag
> is manipulated. I don't know how that is handled in other bridges.
>
> Do we want to include the broadcast flood in the above prototype?
> If we go for this, how do we detect which options a switch supports?
If the necessary dsa_switch_ops routine is correctly prototyped, having it
implemented by a driver or not should be enough to inform the core that the
related feature(s) is/are supported by the switch.
I'll try to give a bit more context on why I'd prefer this approach, hoping
it makes sense: a switch driver does not need to understand bridge flags
per-se, the core should give enough abstraction to this layer (and any other
net-specifics). The core just needs to know if a driver can program this or
that. More importantly, it can easily become messy to maintain switch-cases
of arbitrary flags in all drivers and the core.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Russell King - ARM Linux admin @ 2019-02-19 17:00 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219111612.GF27578@t480s.localdomain>
On Tue, Feb 19, 2019 at 11:16:12AM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> > +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
> > + unsigned long flags)
> > +{
> > + struct mv88e6xxx_chip *chip = ds->priv;
> > + bool unicast, multicast;
> > + int ret = -EOPNOTSUPP;
> > +
> > + unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
> > + flags & BR_FLOOD;
> > + multicast = flags & BR_MCAST_FLOOD;
> > +
> > + mutex_lock(&chip->reg_lock);
> > + if (chip->info->ops->port_set_egress_floods)
> > + ret = chip->info->ops->port_set_egress_floods(chip, port,
> > + unicast,
> > + multicast);
> > + mutex_unlock(&chip->reg_lock);
> > +
> > + return ret;
> > +}
> > +
> > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > +{
> > + struct mv88e6xxx_chip *chip = ds->priv;
> > + unsigned long support = 0;
> > +
> > + if (chip->info->ops->port_set_egress_floods)
> > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > +
> > + return support;
> > +}
>
> I think that it isn't necessary to propagate the notion of bridge flags down
> to the DSA drivers. It might be just enough to add:
>
> port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
>
> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
I've just changed my last patch to set these modes from
dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
I notice this on the ZII rev B board:
At boot (without anything connected to any of the switch ports):
br0: port 1(lan0) entered blocking state
br0: port 1(lan0) entered disabled state
device lan0 entered promiscuous mode
device eth1 entered promiscuous mode
br0: port 2(lan1) entered blocking state
br0: port 2(lan1) entered disabled state
device lan1 entered promiscuous mode
...
I then removed lan0 from the bridge:
device lan0 left promiscuous mode
br0: port 1(lan0) entered disabled state
and then added it back:
br0: port 1(lan0) entered blocking state
br0: port 1(lan0) entered disabled state
device lan0 entered promiscuous mode
Now, you'd expect lan0 and lan1 to be configured the same at this
point, and the same as it was before lan0 was removed from the bridge?
lan0 is port 0, lan1 is port 1 on this switch - and the register debug
says:
GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6
0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04
...
4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f
Note that port 0 is in disabled state, but port 1 and 2 are in
blocking state... but wait, the kernel printed a message saying it was
in disabled state!
If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
expected, and then returns to 0x43c.
It looks like DSA isn't always in sync with bridge as per port state.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: Three questions about busy poll
From: Willem de Bruijn @ 2019-02-19 17:04 UTC (permalink / raw)
To: Cong Wang
Cc: Alexander Duyck, Eric Dumazet, Samudrala, Sridhar,
Linux Kernel Network Developers
In-Reply-To: <CAM_iQpWtg6=qjQ=JFQyEONHujHX0BWEswejtc==RxocHj365uQ@mail.gmail.com>
On Mon, Feb 18, 2019 at 11:30 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Fri, Feb 15, 2019 at 2:47 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > > > > 2. Why there is no socket option for sysctl.net.busy_poll? Clearly
> > > > > sysctl_net_busy_poll is global and SO_BUSY_POLL only works for
> > > > > sysctl.net.busy_read.
> > > >
> > > > I guess because of how sock_poll works. In that case it is not needed.
> > > > The poll duration applies more to the pollset than any of the
> > > > individual sockets, too.
> > >
> > >
> > > Good point, it's probably like struct eventpoll vs. struct epitem.
> > >
> > > The reason why I am looking for a per-socket tuning is to minimize
> > > the impact of setting busy_poll. I don't know if it is possible to somehow
> > > make this per-socket via epoll interfaces, perhaps fundamentally
> > > it is impossible?
> >
> > I think it may be possible. The way busy_read and busy_poll work
> > in sock_poll is that the sum of all (per socket tunable) busy_read
> > durations on the sockets in the pollset is ~bound by (global) busy_poll.
>
>
> Good idea!!
>
> I was actually thinking about checking sk->sk_ll_usec
> ep_set_busy_poll_napi_id(). Your idea sounds better than mine.
> Maybe we can do both together. :)
>
>
> >
> > The epoll implementation is restricted in the sense that it polls only
> > on one napi_id at a time. Alongside setting ep->napi_id in
> > ep_set_busy_poll_napi_id, we could also set a new ep field takes
> > the min of the global busy_poll and sk->sk_ll_usec.
> >
> > Though I guess you want to be able to poll on a given pollset
> > without setting the global sysctl_net_busy_poll at all? That
> > would be a useful feature both for epoll and poll/select. But
> > definitely requires refining net_busy_loop_on() to optionally
> > take some state derived from the sockets in the (e)pollset.
>
> Yes, or at least give some control to each application.
> I was thinking about this:
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index a5d219d920e7..1b104c8bda5e 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -432,7 +432,7 @@ static inline void ep_set_busy_poll_napi_id(struct
> epitem *epi)
> return;
>
> sk = sock->sk;
> - if (!sk)
> + if (!sk || !sk->sk_ll_usec)
> return;
>
> napi_id = READ_ONCE(sk->sk_napi_id);
>
> With this change, busy_poll would rely on either busy_read or
> SO_BUSY_POLL.
>
> Does this make any sense?
The main issue I see would be in doing it unconditionally, changing
existing behavior.
Perhaps we can add a socket flag and replace the e()pollset busy poll
budget with sk->sk_ll_usec if set? The pollsets would need private
fields initialized from sysctl_net_busy_poll. This also allows
selectively enabling busy polling while leaving global
net_core_busy_poll zero otherwise.
This flag could for instance be passed through SO_BUSY_POLL by
accepting an optlen of 2*sizeof(int).
Just a thought. This is quickly getting a lot more complex than a nice
one-line patch.
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Russell King - ARM Linux admin @ 2019-02-19 17:14 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219120000.GB4140@t480s.localdomain>
Hi Vivien,
On Tue, Feb 19, 2019 at 12:00:00PM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Tue, 19 Feb 2019 16:24:35 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > > +{
> > > > + struct mv88e6xxx_chip *chip = ds->priv;
> > > > + unsigned long support = 0;
> > > > +
> > > > + if (chip->info->ops->port_set_egress_floods)
> > > > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > > +
> > > > + return support;
> > > > +}
> > >
> > > I think that it isn't necessary to propagate the notion of bridge flags down
> > > to the DSA drivers. It might be just enough to add:
> > >
> > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> > >
> > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> >
> > There are two other flags that I haven't covered which the bridge code
> > expects to be offloaded, and those are the broadcast flood flag and
> > the learning flag.
>
> I see. What does the bridge code do if these flags are set? Does it expect
> the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
> program this entry into the bridged ports?
The bridge code defaults to all four flags set. See new_nbp() in
net/bridge/br_if.c:
p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's
no man page documentation for that flag.
According to br_flood() in net/bridge/br_forward.c, it controls
whether broadcast frames are flooded to all ports or not. Changing
this flag is merely handled just like the multicast/unicast flooding
flags - a call is made to set the offloaded flags, and if it isn't
returned as being supported, a warning is printed. No attempt is
made to create or change a forwarding entry for the broadcast MAC
address.
bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:
learning on or learning off
Controls whether a given port will learn MAC addresses from
received traffic or not. If learning if off, the bridge will end
up flooding any traffic for which it has no FDB entry. By
default this flag is on.
> In the latter case we have almost nothing to do. In the former case, we can
> make the core call dsa_port_mdb_add on setup and when a VLAN is added.
>
> mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
>
> If tomorrow there's a switch capable of simply toggling a bit to do that,
> we can add a new ops and skip the port_mdb_add call in the core.
>
> > I know that the Marvell switches don't have a bit to control the
> > broadcast flooding, that appears to be controlled via a static entry
> > in the ATU which would have to be modified as the broadcast flood flag
> > is manipulated. I don't know how that is handled in other bridges.
> >
> > Do we want to include the broadcast flood in the above prototype?
> > If we go for this, how do we detect which options a switch supports?
>
> If the necessary dsa_switch_ops routine is correctly prototyped, having it
> implemented by a driver or not should be enough to inform the core that the
> related feature(s) is/are supported by the switch.
>
> I'll try to give a bit more context on why I'd prefer this approach, hoping
> it makes sense: a switch driver does not need to understand bridge flags
> per-se, the core should give enough abstraction to this layer (and any other
> net-specifics). The core just needs to know if a driver can program this or
> that. More importantly, it can easily become messy to maintain switch-cases
> of arbitrary flags in all drivers and the core.
So, should we go the other way and have:
int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);
rather than trying to combine uc/mc into one? It would mean that we'd
be performing more bus reads/writes, but I guess that doesn't matter
for these configuration parameters.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 17:23 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219170059.h5j552kuplvdaqol@shell.armlinux.org.uk>
Hi Russell,
On Tue, 19 Feb 2019 17:00:59 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
>
> I've just changed my last patch to set these modes from
> dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
> I notice this on the ZII rev B board:
>
> At boot (without anything connected to any of the switch ports):
>
> br0: port 1(lan0) entered blocking state
> br0: port 1(lan0) entered disabled state
> device lan0 entered promiscuous mode
> device eth1 entered promiscuous mode
> br0: port 2(lan1) entered blocking state
> br0: port 2(lan1) entered disabled state
> device lan1 entered promiscuous mode
> ...
>
> I then removed lan0 from the bridge:
>
> device lan0 left promiscuous mode
> br0: port 1(lan0) entered disabled state
>
> and then added it back:
>
> br0: port 1(lan0) entered blocking state
> br0: port 1(lan0) entered disabled state
> device lan0 entered promiscuous mode
>
> Now, you'd expect lan0 and lan1 to be configured the same at this
> point, and the same as it was before lan0 was removed from the bridge?
> lan0 is port 0, lan1 is port 1 on this switch - and the register debug
> says:
>
> GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6
> 0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04
> ...
> 4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f
>
> Note that port 0 is in disabled state, but port 1 and 2 are in
> blocking state... but wait, the kernel printed a message saying it was
> in disabled state!
>
> If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
> expected, and then returns to 0x43c.
>
> It looks like DSA isn't always in sync with bridge as per port state.
One thing we have to handle in DSA core are the unbridged ports. Such isolated
ports must be in forwarding state if they are up, so that they can be used
without a bridge, as a standard network interface.
Maybe it'd be simpler if the bridge code would put the interface down when
unbridging it. That way we could remove the dsa_port_set_state_now(dp,
BR_STATE_FORWARDING) hack from dsa_port_bridge_leave. Not sure if that makes
sense though.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH RESEND net] net: phy: xgmiitorgmii: Support generic PHY status read
From: Andrew Lunn @ 2019-02-19 17:25 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Florian Fainelli, netdev, linux-arm-kernel, linux-kernel,
Michal Simek, David S . Miller, Thomas Petazzoni, Heiner Kallweit
In-Reply-To: <b0f700f85cd35bf26a97280f3bef7c4e87102d63.camel@bootlin.com>
> Thanks for the suggestion! So I had a closer look at that driver to try
> and see what could go wrong and it looks like I found a few things
> there.
Hi Paul
Yes, this driver has issues. If i remember correctly, it got merged
while i was on vacation. I pointed out a few issues, but the authors
never responded. Feel free to fix it up.
Andrew
^ permalink raw reply
* Re: [Bridge] [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled
From: Nikolay Aleksandrov @ 2019-02-19 17:26 UTC (permalink / raw)
To: Linus Lüssing; +Cc: bridge, netdev, roopa, f.fainelli, idosch
In-Reply-To: <20190219154219.GG10191@otheros>
On 19/02/2019 17:42, Linus Lüssing wrote:
> On Tue, Feb 19, 2019 at 03:31:42PM +0200, Nikolay Aleksandrov wrote:
>> On 19/02/2019 11:21, Linus Lüssing wrote:
>>> On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote:
>>>> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote:
>>>>> This is v2 of the RFC patch which aims to forward packets to known
>>>>> mdsts' ports only (the no querier case). After v1 I've kept
>>>>> the previous behaviour when it comes to unregistered traffic or when
>>>>> a querier is present. All of this is of course only with snooping
>>>>> enabled. So with this patch the following changes should occur:
>>>>> - No querier: forward known mdst traffic to its registered ports,
>>>>> no change about unknown mcast (flood)
>>>>> - Querier present: no change
>>>>>
>>>>> The reason to do this is simple - we want to respect the user's mdb
>>>>> configuration in both cases, that is if the user adds static mdb entries
>>>>> manually then we should use that information about forwarding traffic.
>>>>>
>>>>> What do you think ?
>>>>>
>>>>> * Notes
>>>>> Traffic that is currently marked as mrouters_only:
>>>>> - IPv4: non-local mcast traffic, igmp reports
>>>>> - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports
>>>>>
>>>>> Simple use case:
>>>>> $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
>>>>> $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
>>>>> - without a querier currently traffic for 239.0.0.1 will still be flooded,
>>>>> with this change it will be forwarded only to swp1
>>>>
>>>> There is still the issue with unsolicited reports adding mdst
>>>> entries here, too. Leading to unwanted packet loss and connectivity issues.
>>>
>>> Or in other words, an unsolicited report will turn a previously
>>> unregistered multicast group into a registered one. However in the
>>> absence of a querier the knowledge about this newly registered multicast group
>>> will be incomplete. And therefore still needs to be flooded to avoid packet
>>> loss.
>>>
>>
>> Right, this is expected. If the user has enabled igmp snooping and doesn't have
>> a querier present then such behaviour is to be expected.
>
> IGMP snooping is currently enabled by default. And IGMP/MLD
> querier is disabled by default. I wouldn't want packet loss to be
> the expected behaviour in a default setup.
>
> This default setup currently works. However with this change it
> will introduce packet loss, as you acknowledged (if I understood
> you correctly?).
>
Yeah, I'm not pushing this change anymore, the only option was to add
a tunable for the behaviour which I described in my previous reply but I
really want to avoid adding yet another bridge option as it has
way too many already.
>> What is surprising is
>> the user explicitly enabling igmp snooping, adding an mdst and then still
>> getting it flooded. :)
>
> Hm, for me that does not seem surprising. I would not expect igmp
> snooping to work without a querier on the link. Why don't you just
> add/enable a querier in your setups then if you want to avoid
> flooding?
>
Yep, ack.
>
>> An alternative is to drop all unregistered traffic when a querier is not present.
>> But that will surely break setups and at best should be a configurable option that
>> is disabled by default.
>
> Absolutely right. Always dropping with no querier is no option. That's why I'd say
> you should always flood multicast packets if there is no querier.
>
>
>> So in effect and to try and make everybody happy we can add an option to control
>> this behaviour with keeping the current as default and adding the following options:
>> - no querier: flood all (default, current)
>
> ACK
>
> For the other options maybe I do not understand your scenario yet.
> Wouldn't these two options result in eratic behaviour?
>
>> - no querier: flood unregistered, forward registered
>> - no querier: drop unregistered, forward registered
>
To be fair I've seen all 3 options being default on different versions of
network OSes by major vendors.
> Let's call these two cases A) - flood unregistered, forward
> registered and B) - drop unregistered, forward registered.
>
>
> Let's say you have a bridge with two ports:
> (1)<-[br]->(2). And no querier.
>
> Behind (2) there is a listener for group M. M is not
> registered by the bridge because either that listener joined
> before the bridge came up or the entry was registered once but had
> timed out. Or there was packet loss and the report did not arrive
> at the bridge (for instance bc. listener is behind a wireless
> connection).
>
> For case B) we can immediately see that the listener at (2) will
> not receive the traffic it signed up for. And this is a permanent
> issue as the listener will not send any further reports.
>
> Case A) is ok, the listener behind port (2) receives its traffic.
>
>
> Now, a listener for M joins at (1). It sends an unsolicited
> report. Group M becomes registered by the bridge. Both for
> cases (A) and (B) this new listener at (1) will receive its
> traffic. However, not only in case B) now, but in case A), too,
> the listener at (2) will rceive no more traffic for M.
>
>
> Now 260 seconds pass (multicast_membership_interval). The entry
> for M times out and is deleted on the bridge. It becomes
> unregistered.
>
> Now for case (A) things would be ok again, both listeners at (1)
> and (2) would receive traffic. For now - as long as no new listener
> joins again.
>
> For case (B), both the listener at port (1) and (2) will fail to
> receive the traffic they signed up for.
>
>
> ---
>
> I hope this illustrates a bit what I'm afraid of? If you have any
> measures to prevent such behavior in your setup, I'd be curious to
> know.
>
It does and I'm aware of these scenarios, but there're static mcast trees
and also such that are controlled by user-space daemons, that is the daemon
keeps refreshing the mdb entries. They're non-standard for sure and obviously
we suggest having a querier always. That's why I suggested having these
as options, but now I'm retracting that and will table this whole thing for
the time being.
I really don't want to add another option to the bridge lightly.
> Regards, Linus
>
Thanks for the discussion and all the helpful comments,
Nik
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Russell King - ARM Linux admin @ 2019-02-19 17:27 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219122304.GB10959@t480s.localdomain>
On Tue, Feb 19, 2019 at 12:23:04PM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Tue, 19 Feb 2019 17:00:59 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> >
> > I've just changed my last patch to set these modes from
> > dsa_port_bridge_join() and dsa_port_bridge_leave(), and while testing,
> > I notice this on the ZII rev B board:
> >
> > At boot (without anything connected to any of the switch ports):
> >
> > br0: port 1(lan0) entered blocking state
> > br0: port 1(lan0) entered disabled state
> > device lan0 entered promiscuous mode
> > device eth1 entered promiscuous mode
> > br0: port 2(lan1) entered blocking state
> > br0: port 2(lan1) entered disabled state
> > device lan1 entered promiscuous mode
> > ...
> >
> > I then removed lan0 from the bridge:
> >
> > device lan0 left promiscuous mode
> > br0: port 1(lan0) entered disabled state
> >
> > and then added it back:
> >
> > br0: port 1(lan0) entered blocking state
> > br0: port 1(lan0) entered disabled state
> > device lan0 entered promiscuous mode
> >
> > Now, you'd expect lan0 and lan1 to be configured the same at this
> > point, and the same as it was before lan0 was removed from the bridge?
> > lan0 is port 0, lan1 is port 1 on this switch - and the register debug
> > says:
> >
> > GLOBAL GLOBAL2 SERDES 0 1 2 3 4 5 6
> > 0: c800 0 1140 500f 500f 500f 500f 500f 4e07 4d04
> > ...
> > 4: 40a8 258 1e0 43c 43d 43d 7c 430 53f 373f
> >
> > Note that port 0 is in disabled state, but port 1 and 2 are in
> > blocking state... but wait, the kernel printed a message saying it was
> > in disabled state!
> >
> > If I do the same for lan1, port 1 above changed from 0x43d to 0x433 as
> > expected, and then returns to 0x43c.
> >
> > It looks like DSA isn't always in sync with bridge as per port state.
>
> One thing we have to handle in DSA core are the unbridged ports. Such isolated
> ports must be in forwarding state if they are up, so that they can be used
> without a bridge, as a standard network interface.
>
> Maybe it'd be simpler if the bridge code would put the interface down when
> unbridging it. That way we could remove the dsa_port_set_state_now(dp,
> BR_STATE_FORWARDING) hack from dsa_port_bridge_leave. Not sure if that makes
> sense though.
I'm not concerned about what happens when the port is removed from the
bridge above.
What I'm concerned about is the two different states when the port is
part of a bridge.
First time, the DSA switch is set to blocking mode, despite the kernel
saying the port is disabled.
Second time, the DSA switch is set to disabled mode, and the kernel
says the port is disabled.
Why is the port in blocking mode first time around?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH][net-next] bridge: remove redundant check on err in br_multicast_ipv4_rcv
From: Roopa Prabhu @ 2019-02-19 17:27 UTC (permalink / raw)
To: Li RongQing; +Cc: netdev, Nikolay Aleksandrov
In-Reply-To: <1550542629-17121-1-git-send-email-lirongqing@baidu.com>
On Mon, Feb 18, 2019 at 6:17 PM Li RongQing <lirongqing@baidu.com> wrote:
>
> br_ip4_multicast_mrd_rcv only return 0 and -ENOMSG,
> no other negative value
>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
looks fine to me. CC Nikolay
> net/bridge/br_multicast.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 4a048fd1cbea..fe9f2d8ca2c1 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1615,12 +1615,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
> if (ip_hdr(skb)->protocol == IPPROTO_PIM)
> br_multicast_pim(br, port, skb);
> } else if (ipv4_is_all_snoopers(ip_hdr(skb)->daddr)) {
> - err = br_ip4_multicast_mrd_rcv(br, port, skb);
> -
> - if (err < 0 && err != -ENOMSG) {
> - br_multicast_err_count(br, port, skb->protocol);
> - return err;
> - }
> + br_ip4_multicast_mrd_rcv(br, port, skb);
> }
>
> return 0;
> --
> 2.16.2
>
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 17:38 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219171414.cvaiw7u2xnd5zk3g@shell.armlinux.org.uk>
Hi Russell,
On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > > > > +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> > > > > +{
> > > > > + struct mv88e6xxx_chip *chip = ds->priv;
> > > > > + unsigned long support = 0;
> > > > > +
> > > > > + if (chip->info->ops->port_set_egress_floods)
> > > > > + support |= BR_FLOOD | BR_MCAST_FLOOD;
> > > > > +
> > > > > + return support;
> > > > > +}
> > > >
> > > > I think that it isn't necessary to propagate the notion of bridge flags down
> > > > to the DSA drivers. It might be just enough to add:
> > > >
> > > > port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
> > > >
> > > > to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
> > > > if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
> > >
> > > There are two other flags that I haven't covered which the bridge code
> > > expects to be offloaded, and those are the broadcast flood flag and
> > > the learning flag.
> >
> > I see. What does the bridge code do if these flags are set? Does it expect
> > the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
> > program this entry into the bridged ports?
>
> The bridge code defaults to all four flags set. See new_nbp() in
> net/bridge/br_if.c:
>
> p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
>
> bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
> userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's
> no man page documentation for that flag.
>
> According to br_flood() in net/bridge/br_forward.c, it controls
> whether broadcast frames are flooded to all ports or not. Changing
> this flag is merely handled just like the multicast/unicast flooding
> flags - a call is made to set the offloaded flags, and if it isn't
> returned as being supported, a warning is printed. No attempt is
> made to create or change a forwarding entry for the broadcast MAC
> address.
OK, thanks for the details. The programming of the broadcast MAC address
must be handled in the core then, I will move this from mv88e6xxx up to the
DSA layer later, but that's totally orthogonal here.
>
> bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:
>
> learning on or learning off
> Controls whether a given port will learn MAC addresses from
> received traffic or not. If learning if off, the bridge will end
> up flooding any traffic for which it has no FDB entry. By
> default this flag is on.
>
> > In the latter case we have almost nothing to do. In the former case, we can
> > make the core call dsa_port_mdb_add on setup and when a VLAN is added.
> >
> > mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
> >
> > If tomorrow there's a switch capable of simply toggling a bit to do that,
> > we can add a new ops and skip the port_mdb_add call in the core.
> >
> > > I know that the Marvell switches don't have a bit to control the
> > > broadcast flooding, that appears to be controlled via a static entry
> > > in the ATU which would have to be modified as the broadcast flood flag
> > > is manipulated. I don't know how that is handled in other bridges.
> > >
> > > Do we want to include the broadcast flood in the above prototype?
> > > If we go for this, how do we detect which options a switch supports?
> >
> > If the necessary dsa_switch_ops routine is correctly prototyped, having it
> > implemented by a driver or not should be enough to inform the core that the
> > related feature(s) is/are supported by the switch.
> >
> > I'll try to give a bit more context on why I'd prefer this approach, hoping
> > it makes sense: a switch driver does not need to understand bridge flags
> > per-se, the core should give enough abstraction to this layer (and any other
> > net-specifics). The core just needs to know if a driver can program this or
> > that. More importantly, it can easily become messy to maintain switch-cases
> > of arbitrary flags in all drivers and the core.
>
> So, should we go the other way and have:
>
> int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
> int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
> int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
> int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);
>
> rather than trying to combine uc/mc into one? It would mean that we'd
> be performing more bus reads/writes, but I guess that doesn't matter
> for these configuration parameters.
I like this very much. As long as these flags can be programmed in switch
devices, these ops totally make sense.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Florian Fainelli @ 2019-02-19 17:44 UTC (permalink / raw)
To: Vivien Didelot, Russell King - ARM Linux admin
Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, netdev
In-Reply-To: <20190219123828.GD10959@t480s.localdomain>
On 2/19/19 9:38 AM, Vivien Didelot wrote:
> Hi Russell,
>
> On Tue, 19 Feb 2019 17:14:14 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
>>>>>> +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
>>>>>> +{
>>>>>> + struct mv88e6xxx_chip *chip = ds->priv;
>>>>>> + unsigned long support = 0;
>>>>>> +
>>>>>> + if (chip->info->ops->port_set_egress_floods)
>>>>>> + support |= BR_FLOOD | BR_MCAST_FLOOD;
>>>>>> +
>>>>>> + return support;
>>>>>> +}
>>>>>
>>>>> I think that it isn't necessary to propagate the notion of bridge flags down
>>>>> to the DSA drivers. It might be just enough to add:
>>>>>
>>>>> port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
>>>>>
>>>>> to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
>>>>> if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
>>>>
>>>> There are two other flags that I haven't covered which the bridge code
>>>> expects to be offloaded, and those are the broadcast flood flag and
>>>> the learning flag.
>>>
>>> I see. What does the bridge code do if these flags are set? Does it expect
>>> the underlying devices to handle ff:ff:ff:ff:ff:ff magically or does it
>>> program this entry into the bridged ports?
>>
>> The bridge code defaults to all four flags set. See new_nbp() in
>> net/bridge/br_if.c:
>>
>> p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
>>
>> bridge(8) doesn't touch BR_BCAST_FLOOD, but it is made available to
>> userspace via netlink and IFLA_BRPORT_BCAST_FLOOD. Hence, there's
>> no man page documentation for that flag.
>>
>> According to br_flood() in net/bridge/br_forward.c, it controls
>> whether broadcast frames are flooded to all ports or not. Changing
>> this flag is merely handled just like the multicast/unicast flooding
>> flags - a call is made to set the offloaded flags, and if it isn't
>> returned as being supported, a warning is printed. No attempt is
>> made to create or change a forwarding entry for the broadcast MAC
>> address.
>
> OK, thanks for the details. The programming of the broadcast MAC address
> must be handled in the core then, I will move this from mv88e6xxx up to the
> DSA layer later, but that's totally orthogonal here.
I am not sure if it makes sense for us to work hard on supporting
BR_BCAST_FLOOD, for instance, on Broadcom switches, there does not
appear to be an easy way to specify whether broadcast traffic will be
flooded or not, it will be. The only way to tsolve that is to create a
MDB/FDB entry with negative logic (e.g.: forward to a
non-existing/connected port). Every other bridge flag typically maps 1:1
with a corresponding hardware feature, so we should support them.
>
>>
>> bridge(8) does document BR_LEARNING via IFLA_BRPORT_LEARNING:
>>
>> learning on or learning off
>> Controls whether a given port will learn MAC addresses from
>> received traffic or not. If learning if off, the bridge will end
>> up flooding any traffic for which it has no FDB entry. By
>> default this flag is on.
>>
>>> In the latter case we have almost nothing to do. In the former case, we can
>>> make the core call dsa_port_mdb_add on setup and when a VLAN is added.
>>>
>>> mv88e6xxx tries to be smart and is already doing that and I'm really not a fan.
>>>
>>> If tomorrow there's a switch capable of simply toggling a bit to do that,
>>> we can add a new ops and skip the port_mdb_add call in the core.
>>>
>>>> I know that the Marvell switches don't have a bit to control the
>>>> broadcast flooding, that appears to be controlled via a static entry
>>>> in the ATU which would have to be modified as the broadcast flood flag
>>>> is manipulated. I don't know how that is handled in other bridges.
>>>>
>>>> Do we want to include the broadcast flood in the above prototype?
>>>> If we go for this, how do we detect which options a switch supports?
>>>
>>> If the necessary dsa_switch_ops routine is correctly prototyped, having it
>>> implemented by a driver or not should be enough to inform the core that the
>>> related feature(s) is/are supported by the switch.
>>>
>>> I'll try to give a bit more context on why I'd prefer this approach, hoping
>>> it makes sense: a switch driver does not need to understand bridge flags
>>> per-se, the core should give enough abstraction to this layer (and any other
>>> net-specifics). The core just needs to know if a driver can program this or
>>> that. More importantly, it can easily become messy to maintain switch-cases
>>> of arbitrary flags in all drivers and the core.
>>
>> So, should we go the other way and have:
>>
>> int (*port_learning)(struct dsa_switch *ds, int port, bool enable);
>> int (*port_egress_flood_uc)(struct dsa_switch *ds, int port, bool enable);
>> int (*port_egress_flood_mc)(struct dsa_switch *ds, int port, bool enable);
>> int (*port_egress_flood_bc)(struct dsa_switch *ds, int port, bool enable);
>>
>> rather than trying to combine uc/mc into one? It would mean that we'd
>> be performing more bus reads/writes, but I guess that doesn't matter
>> for these configuration parameters.
>
> I like this very much. As long as these flags can be programmed in switch
> devices, these ops totally make sense.
No objections or preference here, whatever works :)
--
Florian
^ permalink raw reply
* Re: [PATCH v3] net: ns83820: code cleanup for ns83820_probe_phy()
From: Andrew Lunn @ 2019-02-19 17:46 UTC (permalink / raw)
To: Walter Harms
Cc: Mao Wenan, kernel-janitors, netdev, john.fastabend, hawk,
jakub.kicinski, daniel, ast, julia.lawall
In-Reply-To: <715767253.151509.1550580124264@ox-groupware.bfs.de>
> > for (i=1; i<2; i++) {
>
>
> the loop here seems also pointless, so you can eliminate i.
> (or did i muss something ?)
If you widen out your view a bit, you find all this code is inside a
#ifdef PHY_CODE_IS_FINISHED. I don't see anything which actually
defines that.
So a lot more code could probably be removed.
Andrew
^ permalink raw reply
* Re: L2TPv3 offset
From: Guillaume Nault @ 2019-02-19 17:54 UTC (permalink / raw)
To: James Chapman; +Cc: t.martitz, davem, netdev
In-Reply-To: <e685d22c-e28e-8318-f612-07e2d1cd03ab@katalix.com>
On Tue, Feb 19, 2019 at 04:36:55PM +0000, James Chapman wrote:
> On 19/02/2019 13:09, t.martitz@avm.de wrote:
> >
> > Because everytime a LCCE decapsulates such traffic it'll suffer from
> > unaligned access to the inner IP header (likewise for the outer IP
> > header when encapsulating). It's a fundamental assumption that the IP
> > header is word-aligned in Linux so I'm surprised this isn't solved
> > already. And now the only way "fix" without patching the kernel is
> > being removed.
> >
IIUC, you'd be perfectly fine running without offset if the stack was
properly handling the alignment problems created by the encapsulated
ethernet header.
Unaligned headers is a fundamental problem when tunneling ethernet
frames. I'd expect other encapsulation modules (vxlan, geneve,
gretap...) to have the same issues (but I'd be happy to be proven
wrong).
The topic has been discussed a few years ago:
https://lore.kernel.org/netdev/20160922.015242.735026657310158125.davem@davemloft.net/
I might very well have missed some discussions, but I don't remember
any patch aimed at tackling this problem so far.
^ 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