* [PATCH net-next] net: wan: cosa: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 15:05 UTC (permalink / raw)
To: netdev; +Cc: kas, davem, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in cosa_net_tx_done() when skb
xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/cosa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/cosa.c b/drivers/net/wan/cosa.c
index f6b000d..55f76e4 100644
--- a/drivers/net/wan/cosa.c
+++ b/drivers/net/wan/cosa.c
@@ -769,7 +769,7 @@ static int cosa_net_tx_done(struct channel_data *chan, int size)
chan->netdev->stats.tx_aborted_errors++;
return 1;
}
- dev_kfree_skb_irq(chan->tx_skb);
+ dev_consume_skb_irq(chan->tx_skb);
chan->tx_skb = NULL;
chan->netdev->stats.tx_packets++;
chan->netdev->stats.tx_bytes += size;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next v2 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Leslie Monis @ 2019-02-25 15:04 UTC (permalink / raw)
To: Dave Taht; +Cc: netdev
In-Reply-To: <87ftsc2c1e.fsf@taht.net>
On Mon, Feb 25, 2019 at 06:11:25AM -0800, Dave Taht wrote:
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
> > On 2019-02-25 8:43 a.m., Jamal Hadi Salim wrote:
> >> On 2019-02-25 5:20 a.m., Leslie Monis wrote:
> >>> The current implementation of the PIE queuing discipline is
> >>> according to the
> >>> IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and
> >>> the paper
> >>> [PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
> >>> However, a lot of necessary modifications and enhancements have
> >>> been proposed
> >>> in RFC 8033, which have not yet been incorporated in the source
> >>> code of Linux.
> >>> This patch series helps in achieving the same.
> >>>
> >>> Performance tests carried out using Flent [https://flent.org/]
> >>>
> >>
> >> +Cc the authors of PIE to double check these values.
> >> Please respond (it is how open source works!) and N/ACK
> >
> > Great. Bouncing addresses.
> > D. Taht - can you look at this?
>
> Whose addresses are bouncing? The folk that did pie originally were
> mostly contractors and long ago moved on to other things.
>
> My taht.net address tends to bounce to vger as I long ago mandated
> starttls on all email transactions which is why I use my gmail account
> for postings there. I keep hoping that one day vger will support
> starttls... posting it here as I'm an optimist. I wasn't expecting to be
> cc'd on this submittal....
>
> Pie seems to be mostly an abandoned CISCO effort outside of docsis-pie,
> at least at the moment. I've heard not a peep from them in years.
> (fq_codel seems to have mostly "won" in our world)
>
> I did review all these changes when they went by in v1, and aside from
> the ecn mistake ending up in the final RFC when I wasn't looking[1],
> approve of these changes to sch_pie to make it compliant with the rfc,
> finally.
>
> [1] We proposed refining pie's ecn handling here:
>
> https://github.com/gautamramk/FQ-PIE-for-Linux-Kernel/issues/2
>
> Anyway:
>
> Acked-by: Dave Taht <dave.taht@gmail.com>
>
Hi Dave,
For now we would just like to make the PIE implementation meet the
standards as expressed by the RFC. As ECN handling is outside the
scope of the RFC we can consider refinements to it in the code later.
Thanks,
Leslie
^ permalink raw reply
* [PATCH net-next] net: wan: sbni: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 15:03 UTC (permalink / raw)
To: netdev; +Cc: davem, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in send_complete() when skb
xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/sbni.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/sbni.c b/drivers/net/wan/sbni.c
index 8e8c4c0..40c04ea 100644
--- a/drivers/net/wan/sbni.c
+++ b/drivers/net/wan/sbni.c
@@ -761,7 +761,7 @@ send_complete( struct net_device *dev )
dev->stats.tx_packets++;
dev->stats.tx_bytes += nl->tx_buf_p->len;
#endif
- dev_kfree_skb_irq( nl->tx_buf_p );
+ dev_consume_skb_irq(nl->tx_buf_p);
nl->tx_buf_p = NULL;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next] net: wan: ixp4xx_hss: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 15:02 UTC (permalink / raw)
To: netdev; +Cc: khalasa, davem, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in hss_hdlc_txdone_irq() when
skb xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/ixp4xx_hss.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/ixp4xx_hss.c b/drivers/net/wan/ixp4xx_hss.c
index 6a505c2..5c60dc6 100644
--- a/drivers/net/wan/ixp4xx_hss.c
+++ b/drivers/net/wan/ixp4xx_hss.c
@@ -246,7 +246,7 @@
#ifdef __ARMEB__
typedef struct sk_buff buffer_t;
#define free_buffer dev_kfree_skb
-#define free_buffer_irq dev_kfree_skb_irq
+#define free_buffer_irq dev_consume_skb_irq
#else
typedef void buffer_t;
#define free_buffer kfree
--
2.7.4
^ permalink raw reply related
* [PATCH net-next] net: wan: wanxl: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 15:01 UTC (permalink / raw)
To: netdev; +Cc: khc, davem, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in wanxl_tx_intr() when skb
xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/wanxl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/wanxl.c b/drivers/net/wan/wanxl.c
index d573a57..5c234a6 100644
--- a/drivers/net/wan/wanxl.c
+++ b/drivers/net/wan/wanxl.c
@@ -185,7 +185,7 @@ static inline void wanxl_tx_intr(struct port *port)
desc->stat = PACKET_EMPTY; /* Free descriptor */
pci_unmap_single(port->card->pdev, desc->address, skb->len,
PCI_DMA_TODEVICE);
- dev_kfree_skb_irq(skb);
+ dev_consume_skb_irq(skb);
port->tx_in = (port->tx_in + 1) % TX_BUFFERS;
}
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next] net: lmc: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 14:57 UTC (permalink / raw)
To: netdev; +Cc: davem, colin.king, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in lmc_interrupt() when skb
xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Delete a redundant comment line in lmc_interrupt().
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/lmc/lmc_main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wan/lmc/lmc_main.c b/drivers/net/wan/lmc/lmc_main.c
index 4907453..9a8e809 100644
--- a/drivers/net/wan/lmc/lmc_main.c
+++ b/drivers/net/wan/lmc/lmc_main.c
@@ -1320,8 +1320,7 @@ static irqreturn_t lmc_interrupt (int irq, void *dev_instance) /*fold00*/
sc->lmc_device->stats.tx_packets++;
}
- // dev_kfree_skb(sc->lmc_txq[i]);
- dev_kfree_skb_irq(sc->lmc_txq[i]);
+ dev_consume_skb_irq(sc->lmc_txq[i]);
sc->lmc_txq[i] = NULL;
badtx++;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2 2/2] drivers: net: phy: mdio-mux: Add support for Generic Mux controls
From: Peter Rosin @ 2019-02-25 14:56 UTC (permalink / raw)
To: Andrew Lunn, Pankaj Bansal
Cc: Leo Li, Florian Fainelli, Heiner Kallweit, netdev@vger.kernel.org
In-Reply-To: <20190224145733.GD26626@lunn.ch>
On 2019-02-24 15:57, Andrew Lunn wrote:
>> +static int mdio_mux_multiplexer_remove(struct platform_device *pdev)
>> +{
>> + struct mdio_mux_multiplexer_state *s = platform_get_drvdata(pdev);
>> +
>> + mdio_mux_uninit(s->mux_handle);
>
> I've never used the multiplexer framework before. But i think you need
> a call to mux_control_deselect() here in order to unlock the
> multiplexer. Without that, it will deadlock when you reload the
> module.
That is correct.
Some background; The mux framework was designed for cases where
a) two (or more) mux consumers "compete" for the same mux controller, or
b) where a single mux chip shares several mux controllers where each
controller might be used for some independent purpose.
The design is also built around short accesses, such that no single
mux consumer starves other consumers of a shared mux controller. I.e.
- select the mux and implicitly set it in the desired position
- do something short-ish which requires the mux
- deselect the mux
But it does work for exclusive consumers of a mux controller as in this
series. However, if the consumer
1) requires the implementation of a new mux controller,
2) does not need any of the existing mux drivers and
3) the situation is not in one of the a) and b) categories
then the mux framework is not really providing any killer features. I.e.
it seems like it is certainly an option to just open code the needed
regmap accesses and ignore the multiplexer subsystem in this case.
However, the changes needed in the mux framework are quite minimal in
this case [1], so 1) is arguably not applicable...
More details on the design can be found in the original mux series [2].
Cheers,
Peter
[1] https://lkml.org/lkml/2019/2/24/23
[2] https://lkml.org/lkml/2017/5/14/160
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Marc Zyngier @ 2019-02-25 14:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, Devicetree List, <netdev@vger.kernel.org>,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
linux-rockchip, David S. Miller, linux-arm-kernel
In-Reply-To: <CAKv+Gu9OOzOxx0rTAZqM8r4q_hRmOp3b=KP7fwgNeiu67FCApA@mail.gmail.com>
Hi Ard,
On 25/02/2019 12:45, Ard Biesheuvel wrote:
> On Sun, 24 Feb 2019 at 15:08, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>
>> For quite some time, I wondered why the PCI mwifiex device built in my
>> Chromebook was unable to use the good old legacy interrupts. But as MSIs
>> were working fine, I never really bothered investigating. I finally had a
>> look, and the result isn't very pretty.
>>
>> On this machine (rk3399-based kevin), the wake-up interrupt is described as
>> such:
>>
>> &pci_rootport {
>> mvl_wifi: wifi@0,0 {
>> compatible = "pci1b4b,2b42";
>> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
>> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
>> interrupt-parent = <&gpio0>;
>> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
>> pinctrl-names = "default";
>> pinctrl-0 = <&wlan_host_wake_l>;
>> wakeup-source;
>> };
>> };
>>
>> Note how the interrupt is part of the properties directly attached to the
>> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
>> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
>> altogether (Yay for the broken design!). This is in total violation of the
>> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
>> specifiers describe the PCI device interrupts, and must obey the
>> INT-{A,B,C,D} mapping. Oops!
>>
>
> The mapping of legacy PCIe INTx interrupts onto wired system
> interrupts is a property of the PCIe host controller, not of a
> particular PCIe device. So I would argue that the code is broken here
> as well: it should never attempt to interpret 'interrupt' properties
> at the PCI device level as having any bearing on how legacy interrupts
> are routed.
OpenFirmware says that this node contains the interrupt number of the
device (4.1.1. Open Firmware-defined Properties for Child Nodes). The
trick is that this property is generated *from* the device, and not set
in stone.
DT, on the other hand, takes whatever is described there and uses it as
the gospel to configure the OS, no matter how the PCI device is actually
configured. If the two don't match (like in this case), things break.
This is the "DT describes the HW" mantra, for (sometimes) better or
(more generally) worse.
What the DT code does is to interpret the whole interrupt specifier,
*including the interrupt-parent*. And that feels wrong. It should always
be in the context of the host controller. But on the other side, the DT
code is not in the business of validating the DT either...
It outlines one thing: If you have to interpret per-device PCI
properties from DT, you're in for serious trouble. I should get some
better HW.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
From: Sheng Lan @ 2019-02-25 14:49 UTC (permalink / raw)
To: davem, stephen
Cc: netdev, netem, xuhanbing, zhengshaoyu, jiqin.ji, liuzhiqiang26
From: Sheng Lan <lansheng@huawei.com>
Subject: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
It can be reproduced by following steps:
1. virtio_net NIC is configured with gso/tso on
2. configure nginx as http server with an index file bigger than 1M bytes
3. use tc netem to produce duplicate packets and delay:
tc qdisc add dev eth0 root netem delay 100ms 10ms 30% duplicate 90%
4. continually curl the nginx http server to get index file on client
5. BUG_ON is seen quickly
[10258690.371129] kernel BUG at net/core/skbuff.c:4028!
[10258690.371748] invalid opcode: 0000 [#1] SMP PTI
[10258690.372094] CPU: 5 PID: 0 Comm: swapper/5 Tainted: G W 5.0.0-rc6 #2
[10258690.372094] RSP: 0018:ffffa05797b43da0 EFLAGS: 00010202
[10258690.372094] RBP: 00000000000005ea R08: 0000000000000000 R09: 00000000000005ea
[10258690.372094] R10: ffffa0579334d800 R11: 00000000000002c0 R12: 0000000000000002
[10258690.372094] R13: 0000000000000000 R14: ffffa05793122900 R15: ffffa0578f7cb028
[10258690.372094] FS: 0000000000000000(0000) GS:ffffa05797b40000(0000) knlGS:0000000000000000
[10258690.372094] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[10258690.372094] CR2: 00007f1a6dc00868 CR3: 000000001000e000 CR4: 00000000000006e0
[10258690.372094] Call Trace:
[10258690.372094] <IRQ>
[10258690.372094] skb_to_sgvec+0x11/0x40
[10258690.372094] start_xmit+0x38c/0x520 [virtio_net]
[10258690.372094] dev_hard_start_xmit+0x9b/0x200
[10258690.372094] sch_direct_xmit+0xff/0x260
[10258690.372094] __qdisc_run+0x15e/0x4e0
[10258690.372094] net_tx_action+0x137/0x210
[10258690.372094] __do_softirq+0xd6/0x2a9
[10258690.372094] irq_exit+0xde/0xf0
[10258690.372094] smp_apic_timer_interrupt+0x74/0x140
[10258690.372094] apic_timer_interrupt+0xf/0x20
[10258690.372094] </IRQ>
In __skb_to_sgvec, the skb->len is not equal to the sum of the skb's
linear data size and nonlinear data size, thus BUG_ON triggered. The
bad skb's nonlinear data size is less than skb->data_len, because the
skb is cloned and a part of related cloned skb's nonlinear data is
split off.
Duplicate packet is cloned by skb_clone in netem_enqueue and may be delayed
some time in qdisc. Due to the delay time, the original skb will be pushed
again later in __tcp_push_pending_frames when tcp receives new packets.
In tcp_write_xmit, when the tcp_mss_split_point returns a smaller limit,
the original skb will be fragmented and the skb's nonlinear data will be
split off. The length of the skb cloned by netem will not be updated.
When we use virtio_net NIC, the duplicated cloned skb will be filled into
a scatter-gather list in __skb_to_sgvec and trigger the BUG_ON.
Here I replace the skb_clone with skb_copy in netem_enqueue to ensure
the duplicated skb's nonlinear data is independent.
Signed-off-by: Sheng Lan <lansheng@huawei.com>
Reported-by: Qin Ji <jiqin.ji@huawei.com>
Fixes: 0afb51e7 ("netem: reinsert for duplication")
---
net/sched/sch_netem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 75046ec..76d9740 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -479,7 +479,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
* qdisc tree, since parent queuer expects that only one
* skb will be queued.
*/
- if (count > 1 && (skb2 = skb_clone(skb, GFP_ATOMIC)) != NULL) {
+ if (count > 1 && (skb2 = skb_copy(skb, GFP_ATOMIC)) != NULL) {
struct Qdisc *rootq = qdisc_root(sch);
u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
--
^ permalink raw reply related
* Re: [net-next 05/14] i40e: fix up 32 bit timespec references
From: Arnd Bergmann @ 2019-02-25 14:46 UTC (permalink / raw)
To: Jeff Kirsher
Cc: David Miller, Jesse Brandeburg, Networking, nhorman, sassmann,
jogreene
In-Reply-To: <20170726103123.76743-6-jeffrey.t.kirsher@intel.com>
On Wed, Jul 26, 2017 at 12:33 PM Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
>
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
>
> As it turns out there was only a small set of errors
> on 32 bit, and we just needed to be using the right calls
> for dealing with timespec64 variables.
I just stumbled over code added by this older patch, and can't make sense
of the commit description here. Was this an attempt to fix a bug, or
just a cleanup?
>
> - then = ns_to_timespec64(delta);
> mutex_lock(&pf->tmreg_lock);
>
> i40e_ptp_read(pf, &now);
> - now = timespec64_add(now, then);
> + timespec64_add_ns(&now, delta);
> i40e_ptp_write(pf, (const struct timespec64 *)&now);
The problem I noticed here is that 'delta' is a user provided 64-bit
number from clock_adjtime(), and timespec64_add_ns() performs uses
a repeated addition instead of a div/mod pair. When the number
is large, we may end up adding a single second 8 billion times,
which may take a while even on a fast CPU.
Should the commit 0ac30ce43323 ("i40e: fix up 32 bit timespec
references") just be reverted?
Arnd
^ permalink raw reply
* Re: [PATCH net-next v2 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Jamal Hadi Salim @ 2019-02-25 14:33 UTC (permalink / raw)
To: Dave Taht; +Cc: Leslie Monis, netdev, dave.taht
In-Reply-To: <87ftsc2c1e.fsf@taht.net>
On 2019-02-25 9:11 a.m., Dave Taht wrote:
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
>> On 2019-02-25 8:43 a.m., Jamal Hadi Salim wrote:
>>> On 2019-02-25 5:20 a.m., Leslie Monis wrote:
>>>> The current implementation of the PIE queuing discipline is
>>>> according to the
>>>> IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and
>>>> the paper
>>>> [PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
>>>> However, a lot of necessary modifications and enhancements have
>>>> been proposed
>>>> in RFC 8033, which have not yet been incorporated in the source
>>>> code of Linux.
>>>> This patch series helps in achieving the same.
>>>>
>>>> Performance tests carried out using Flent [https://flent.org/]
>>>>
>>>
>>> +Cc the authors of PIE to double check these values.
>>> Please respond (it is how open source works!) and N/ACK
>>
>> Great. Bouncing addresses.
>> D. Taht - can you look at this?
>
> Whose addresses are bouncing? The folk that did pie originally were
> mostly contractors and long ago moved on to other things.
>
The Cisco addresses - listed as authors of PIE were bouncing.
Note: they are still listed as authors of the referenced
IETF draft with those email addresses.
> My taht.net address tends to bounce to vger as I long ago mandated
> starttls on all email transactions which is why I use my gmail account
> for postings there. I keep hoping that one day vger will support
> starttls... posting it here as I'm an optimist. I wasn't expecting to be
> cc'd on this submittal....
>
> Pie seems to be mostly an abandoned CISCO effort outside of docsis-pie,
> at least at the moment. I've heard not a peep from them in years.
> (fq_codel seems to have mostly "won" in our world)
>
> I did review all these changes when they went by in v1, and aside from
> the ecn mistake ending up in the final RFC when I wasn't looking[1],
> approve of these changes to sch_pie to make it compliant with the rfc,
> finally.
>
> [1] We proposed refining pie's ecn handling here:
>
> https://github.com/gautamramk/FQ-PIE-for-Linux-Kernel/issues/2
>
> Anyway:
>
> Acked-by: Dave Taht <dave.taht@gmail.com>
>
Thats what i was hoping for. FWIW,
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next v2 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Dave Taht @ 2019-02-25 14:11 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: Leslie Monis, netdev, dave.taht
In-Reply-To: <19575745-ad10-be2d-6259-5fe39fa71264@mojatatu.com>
Jamal Hadi Salim <jhs@mojatatu.com> writes:
> On 2019-02-25 8:43 a.m., Jamal Hadi Salim wrote:
>> On 2019-02-25 5:20 a.m., Leslie Monis wrote:
>>> The current implementation of the PIE queuing discipline is
>>> according to the
>>> IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and
>>> the paper
>>> [PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
>>> However, a lot of necessary modifications and enhancements have
>>> been proposed
>>> in RFC 8033, which have not yet been incorporated in the source
>>> code of Linux.
>>> This patch series helps in achieving the same.
>>>
>>> Performance tests carried out using Flent [https://flent.org/]
>>>
>>
>> +Cc the authors of PIE to double check these values.
>> Please respond (it is how open source works!) and N/ACK
>
> Great. Bouncing addresses.
> D. Taht - can you look at this?
Whose addresses are bouncing? The folk that did pie originally were
mostly contractors and long ago moved on to other things.
My taht.net address tends to bounce to vger as I long ago mandated
starttls on all email transactions which is why I use my gmail account
for postings there. I keep hoping that one day vger will support
starttls... posting it here as I'm an optimist. I wasn't expecting to be
cc'd on this submittal....
Pie seems to be mostly an abandoned CISCO effort outside of docsis-pie,
at least at the moment. I've heard not a peep from them in years.
(fq_codel seems to have mostly "won" in our world)
I did review all these changes when they went by in v1, and aside from
the ecn mistake ending up in the final RFC when I wasn't looking[1],
approve of these changes to sch_pie to make it compliant with the rfc,
finally.
[1] We proposed refining pie's ecn handling here:
https://github.com/gautamramk/FQ-PIE-for-Linux-Kernel/issues/2
Anyway:
Acked-by: Dave Taht <dave.taht@gmail.com>
^ permalink raw reply
* Re: [PATCH net] sctp: get sctphdr by offset in sctp_compute_cksum
From: Neil Horman @ 2019-02-25 14:07 UTC (permalink / raw)
To: Xin Long; +Cc: LKML, network dev, linux-sctp, davem, Marcelo Ricardo Leitner
In-Reply-To: <CADvbK_cC4q1D5w1AHAVQjCTW4Pm6H+-ZOVg7xCp+eiOkkRJ8SQ@mail.gmail.com>
On Mon, Feb 25, 2019 at 09:20:44PM +0800, Xin Long wrote:
> On Mon, Feb 25, 2019 at 8:47 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Mon, Feb 25, 2019 at 07:25:37PM +0800, Xin Long wrote:
> > > sctp_hdr(skb) only works when skb->transport_header is set properly.
> > >
> > > But in the path of nf_conntrack_in: sctp_packet() -> sctp_error()
> > >
> > > skb->transport_header is not guaranteed to be right value for sctp.
> > > It will cause to fail to check the checksum for sctp packets.
> > >
> > > So fix it by using offset, which is always right in all places.
> > >
> > > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > > Reported-by: Li Shuang <shuali@redhat.com>
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > > include/net/sctp/checksum.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
> > > index 32ee65a..1c6e6c0 100644
> > > --- a/include/net/sctp/checksum.h
> > > +++ b/include/net/sctp/checksum.h
> > > @@ -61,7 +61,7 @@ static inline __wsum sctp_csum_combine(__wsum csum, __wsum csum2,
> > > static inline __le32 sctp_compute_cksum(const struct sk_buff *skb,
> > > unsigned int offset)
> > > {
> > > - struct sctphdr *sh = sctp_hdr(skb);
> > > + struct sctphdr *sh = (struct sctphdr *)(skb->data + offset);
> > > const struct skb_checksum_ops ops = {
> > > .update = sctp_csum_update,
> > > .combine = sctp_csum_combine,
> > > --
> > > 2.1.0
> > >
> > >
> > Shouldn't you use skb_set_transport_header and skb_transport_header here?
> you mean:
> skb_set_transport_header(skb, offset);
> sh = sctp_hdr(skb);
> ?
>
> There's no place counting on here to set transport_header.
> It will be a kinda redundant job, yet skb is 'const'.
>
I'm not sure what you mean by "theres no place counting here". We have the
transport header offset, and you're doing the exact same computation that that
function does. It seems like we should use it in case the underlying
implementation changes.
I understand what you are saying regarding the use of a const variable there,
but perhaps thats an argument for removing the const storage classifier. Better
still, it would be good to figure out why all paths to this function don't
already set the transport header offset to begin with (addressing your redundant
comment)
Regards
Neil
^ permalink raw reply
* Re: [PATCH net-next] net: sched: act_tunnel_key: fix metadata handling
From: Davide Caratti @ 2019-02-25 14:04 UTC (permalink / raw)
To: Vlad Buslov, netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, wenxu, roid
In-Reply-To: <20190225122122.8128-1-vladbu@mellanox.com>
On Mon, 2019-02-25 at 14:21 +0200, Vlad Buslov wrote:
> Tunnel key action params->tcft_enc_metadata is only set when action is
> TCA_TUNNEL_KEY_ACT_SET. However, metadata pointer is incorrectly
> dereferenced during tunnel key init and release without verifying that
> action is if correct type, which causes NULL pointer dereference. Metadata
> tunnel dst_cache is also leaked on action overwrite.
>
> Fix metadata handling:
> - Verify that metadata pointer is not NULL before dereferencing it in
> tunnel_key_init error handling code.
hello Vlad,
thanks a lot for fixing this!
<...>
> @@ -384,10 +390,12 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>
> release_dst_cache:
> #ifdef CONFIG_DST_CACHE
> - dst_cache_destroy(&metadata->u.tun_info.dst_cache);
> + if (metadata)
> + dst_cache_destroy(&metadata->u.tun_info.dst_cache);
> #endif
> release_tun_meta:
> - dst_release(&metadata->dst);
> + if (metadata)
> + dst_release(&metadata->dst);
on Linux 'net' tree we don't have commit 41411e2fd6b8 ("net/sched:
act_tunnel_key: Add dst_cache support"), but still the above two lines can
avoid a NULL dereference in tunnel_key_init() error path, in the following
case:
* create an action with tunnel "set", with success
* replace the previous rule rule with tunnel "unset", and have a failure
here (e.g. allocation of 'params_new').
At the cost of creating some conflicts during the merge, it would probably
be safer to split this commit into two parts, one targeting 'net' and one
targeting 'net-next', so that the first one can be proposed for stable
backports (and also I can rebase/retest my 'goto chain' series on top of
it :) )
WDYT?
--
davide
^ permalink raw reply
* Re: Fw: [Bug 202651] New: Thunderbolt: Regression in atlantic.ko between 4.20 and 5.00-rc4 for Aquantia NIC 1d6a:87b1
From: Igor Russkikh @ 2019-02-25 14:03 UTC (permalink / raw)
To: Stephen Hemminger, netdev@vger.kernel.org
Cc: Dmitry Bogdanov, linux-kernel@vger.kernel.org
In-Reply-To: <20190223085055.04357494@shemminger-XPS-13-9360>
Hi Stephen,
Thanks for heads up.
We are now looking into this, but the strange thing is atlantic driver had no valuable changes
between 4.20 and 5.0rc.
Also, from what user writes up in the ticket, the issue seems to be related with pci/thunderbolt
subsystem, not with the device driver itself: after couple of hot plugs/unplugs system hangs.
Will try to reproduce that ASAP on our hardware.
Regards,
Igor
On 23.02.2019 19:50, Stephen Hemminger wrote:
>
>
> Begin forwarded message:
>
> Date: Sat, 23 Feb 2019 09:01:29 +0000
> From: bugzilla-daemon@bugzilla.kernel.org
> To: stephen@networkplumber.org
> Subject: [Bug 202651] New: Regression in atlantic.ko between 4.20 and 5.00-rc4 for Aquantia NIC 1d6a:87b1
>
>
> https://bugzilla.kernel.org/show_bug.cgi?id=202651
>
> Bug ID: 202651
> Summary: Regression in atlantic.ko between 4.20 and 5.00-rc4
> for Aquantia NIC 1d6a:87b1
> Product: Networking
> Version: 2.5
> Kernel Version: 5.00-rc4
> Hardware: All
> OS: Linux
> Tree: Mainline
> Status: NEW
> Severity: high
> Priority: P1
> Component: IPV4
> Assignee: stephen@networkplumber.org
> Reporter: nicholas.johnson@outlook.com.au
> Regression: No
>
> Hardware Details:
> ==============================================================================
>
> VEN:DEV = 1d6a:87b1 "Aquantia Corp. AQC107 NBase-T/IEEE 802.3bz Ethernet
> Controller [AQtion]" Specifically, this is the newer AQC107S with "S"
> meaning "Secure" and the company could never elaborate on how it is more
> secure than the plain AQC107 (different VEN:DEV). This chip is inside
> the Promise SANLink3 N1 Thunderbolt 3 10GbE adapter.
>
>
> Severity:
> ==============================================================================
>
> Marked high severity because it directly impacts user experience (cannot
> get the network to come up on the adapter).
>
>
> Back Story and Problem:
> ==============================================================================
>
> In 5.00-rc4, on my Ubuntu desktop I have had extreme problems getting
> the connection up (borderline impossible). If I did manage (after
> messing with systemd *a lot*) then I got a dodgy connection with a lot
> of DNS resolution problems. At first I blamed it on my desktop which had
> a habit of corrupting the SSD partition tables on a regular basis, and
> hence I assumed that systemd was corrupted or something.
>
> Recently, I tried on my laptop with Arch Linux and 5.00-rc4 and it had
> the exact same problem. It does not matter if you manually assign the
> IPv4 address, or use DHCPv4.
>
> Just now, on my laptop, I booted into 4.20.3-arch1-1-ARCH kernel from
> Arch Linux and it worked straight away.
>
>
> Expected behaviour:
> ==============================================================================
>
> NetworkManager (or any tool) should bring the link up successfully.
>
>
> Reproduction:
> ==============================================================================
>
> Should be reproducible with a 5.00-rc4 kernel and the 1d6a:87b1 NIC. I
> have no way of testing if other Aquantia devices are affected - I do not
> own any others. I am using Ubuntu and Arch (both the very latest) and I
> just cannot get NetworkManager to bring up the connection with 5.00-rc4
> (happens effortlessly with 4.20 kernel). If you want NetworkManager to
> manage the adapter, you may need to use the
> 10-globally-managed-devices.conf fix, depending on the version.
>
>
> Technical Notes:
> ==============================================================================
>
> It is hard to tell, but my gut tells me that it is IPv4-related. That
> would explain why some sites could work. I have a native IPv6 service
> from my ISP. The "ifconfig" command shows IPv6 addresses assigned, but
> it is very difficult to get a DHCPv4-assigned address on the adapter.
>
> A couple of times I did manage to get the adapter up in 5.00-rc4 with a
> lot of messing around, and the resulting internet connection was
> unreliable and unstable. DNS resolution seemed to be a big part of the
> problem.
>
> All BARs are assigned correctly. Other than that, dmesg does not give
> anything interesting that I can tell, so if you want some debug logs
> from somewhere then please tell me what you want and how I should do it.
>
> There are a fair few lines of diff between 4.20 and 5.00-rc4 so there is
> likely an oopsie in there.
>
>
> Always Remembering My Manners:
> ==============================================================================
>
> Thank you for reading my bug report!
>
>
> Why Are Web Developers So Annoying?
> ==============================================================================
>
> The preview removes all my newlines and nice formatting which I did so have fun
> trying to read this big blob of unformatted text.
>
^ permalink raw reply
* Re: [PATCH net-next v2 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Jamal Hadi Salim @ 2019-02-25 13:47 UTC (permalink / raw)
To: Leslie Monis; +Cc: netdev, dave
In-Reply-To: <e39403e3-4859-b7f1-b9cc-a8ff85864a9d@mojatatu.com>
On 2019-02-25 8:43 a.m., Jamal Hadi Salim wrote:
> On 2019-02-25 5:20 a.m., Leslie Monis wrote:
>> The current implementation of the PIE queuing discipline is according
>> to the
>> IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and the
>> paper
>> [PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
>> However, a lot of necessary modifications and enhancements have been
>> proposed
>> in RFC 8033, which have not yet been incorporated in the source code
>> of Linux.
>> This patch series helps in achieving the same.
>>
>> Performance tests carried out using Flent [https://flent.org/]
>>
>
> +Cc the authors of PIE to double check these values.
> Please respond (it is how open source works!) and N/ACK
Great. Bouncing addresses.
D. Taht - can you look at this?
cheers,
jamal
^ permalink raw reply
* Re: [PATCH v2] xfrm: correctly check policy index in verify_newpolicy_info
From: Herbert Xu @ 2019-02-25 13:43 UTC (permalink / raw)
To: Yue Haibing; +Cc: steffen.klassert, davem, linux-kernel, netdev
In-Reply-To: <20190225095600.26260-1-yuehaibing@huawei.com>
On Mon, Feb 25, 2019 at 05:56:00PM +0800, Yue Haibing wrote:
>
> the check. Then __xfrm_policy_unlink use the index to access array policy_count
> whose size is XFRM_POLICY_MAX * 2, triggering out of bounds access.
No it doesn't. Even if it did the bug would be in __xfrm_policy_unlink
and not here.
Your patch makes no sense.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH net-next v2 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Jamal Hadi Salim @ 2019-02-25 13:43 UTC (permalink / raw)
To: Leslie Monis; +Cc: netdev, dave, vijaynsu, mysuryan
In-Reply-To: <20190225102051.12268-1-lesliemonis@gmail.com>
On 2019-02-25 5:20 a.m., Leslie Monis wrote:
> The current implementation of the PIE queuing discipline is according to the
> IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and the paper
> [PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
> However, a lot of necessary modifications and enhancements have been proposed
> in RFC 8033, which have not yet been incorporated in the source code of Linux.
> This patch series helps in achieving the same.
>
> Performance tests carried out using Flent [https://flent.org/]
>
+Cc the authors of PIE to double check these values.
Please respond (it is how open source works!) and N/ACK
cheers,
jamal
^ permalink raw reply
* Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
From: Masami Hiramatsu @ 2019-02-25 13:36 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jann Horn, Nadav Amit, Alexei Starovoitov, Steven Rostedt,
Linus Torvalds, Masami Hiramatsu, Linux List Kernel Mailing,
Ingo Molnar, Andrew Morton, Changbin Du, Kees Cook,
Daniel Borkmann, Network Development, bpf@vger.kernel.org,
Rick Edgecombe, Dave Hansen, Peter Zijlstra (Intel), Igor Stoppa
In-Reply-To: <CALCETrU2V5KBA97g3O-yHiUu1acmM_K9b2a5ATmKSqGwdd-+Dw@mail.gmail.com>
On Fri, 22 Feb 2019 15:59:30 -0800
Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Feb 22, 2019 at 3:02 PM Jann Horn <jannh@google.com> wrote:
> >
> > On Fri, Feb 22, 2019 at 11:39 PM Nadav Amit <namit@vmware.com> wrote:
> > > > On Feb 22, 2019, at 2:21 PM, Nadav Amit <namit@vmware.com> wrote:
> > > >
> > > >> On Feb 22, 2019, at 2:17 PM, Jann Horn <jannh@google.com> wrote:
> > > >>
> > > >> On Fri, Feb 22, 2019 at 11:08 PM Nadav Amit <namit@vmware.com> wrote:
> > > >>>> On Feb 22, 2019, at 1:43 PM, Jann Horn <jannh@google.com> wrote:
> > > >>>>
> > > >>>> (adding some people from the text_poke series to the thread, removing stable@)
> > > >>>>
> > > >>>> On Fri, Feb 22, 2019 at 8:55 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > > >>>>>> On Feb 22, 2019, at 11:34 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > > >>>>>>> On Fri, Feb 22, 2019 at 02:30:26PM -0500, Steven Rostedt wrote:
> > > >>>>>>> On Fri, 22 Feb 2019 11:27:05 -0800
> > > >>>>>>> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > > >>>>>>>
> > > >>>>>>>>> On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote:
> > > >>>>>>>>>
> > > >>>>>>>>> Then we should still probably fix up "__probe_kernel_read()" to not
> > > >>>>>>>>> allow user accesses. The easiest way to do that is actually likely to
> > > >>>>>>>>> use the "unsafe_get_user()" functions *without* doing a
> > > >>>>>>>>> uaccess_begin(), which will mean that modern CPU's will simply fault
> > > >>>>>>>>> on a kernel access to user space.
> > > >>>>>>>>
> > > >>>>>>>> On bpf side the bpf_probe_read() helper just calls probe_kernel_read()
> > > >>>>>>>> and users pass both user and kernel addresses into it and expect
> > > >>>>>>>> that the helper will actually try to read from that address.
> > > >>>>>>>>
> > > >>>>>>>> If __probe_kernel_read will suddenly start failing on all user addresses
> > > >>>>>>>> it will break the expectations.
> > > >>>>>>>> How do we solve it in bpf_probe_read?
> > > >>>>>>>> Call probe_kernel_read and if that fails call unsafe_get_user byte-by-byte
> > > >>>>>>>> in the loop?
> > > >>>>>>>> That's doable, but people already complain that bpf_probe_read() is slow
> > > >>>>>>>> and shows up in their perf report.
> > > >>>>>>>
> > > >>>>>>> We're changing kprobes to add a specific flag to say that we want to
> > > >>>>>>> differentiate between kernel or user reads. Can this be done with
> > > >>>>>>> bpf_probe_read()? If it's showing up in perf report, I doubt a single
> > > >>>>>>
> > > >>>>>> so you're saying you will break existing kprobe scripts?
> > > >>>>>> I don't think it's a good idea.
> > > >>>>>> It's not acceptable to break bpf_probe_read uapi.
> > > >>>>>
> > > >>>>> If so, the uapi is wrong: a long-sized number does not reliably identify an address if you don’t separately know whether it’s a user or kernel address. s390x and 4G:4G x86_32 are the notable exceptions. I have lobbied for RISC-V and future x86_64 to join the crowd. I don’t know whether I’ll win this fight, but the uapi will probably have to change for at least s390x.
> > > >>>>>
> > > >>>>> What to do about existing scripts is a different question.
> > > >>>>
> > > >>>> This lack of logical separation between user and kernel addresses
> > > >>>> might interact interestingly with the text_poke series, specifically
> > > >>>> "[PATCH v3 05/20] x86/alternative: Initialize temporary mm for
> > > >>>> patching" (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-6-rick.p.edgecombe%40intel.com%2F&data=02%7C01%7Cnamit%40vmware.com%7Cf2513009ef734ecd6b0d08d69913a5ae%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864707020821793&sdata=HAbnDcrBne64JyPuVUMKmM7nQk67F%2BFvjuXEn8TmHeo%3D&reserved=0)
> > > >>>> and "[PATCH v3 06/20] x86/alternative: Use temporary mm for text
> > > >>>> poking" (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-7-rick.p.edgecombe%40intel.com%2F&data=02%7C01%7Cnamit%40vmware.com%7Cf2513009ef734ecd6b0d08d69913a5ae%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864707020821793&sdata=vNRIMKtFDy%2F3z5FlTwDiJY6VGEV%2FMHgQPTdFSFtCo4s%3D&reserved=0),
> > > >>>> right? If someone manages to get a tracing BPF program to trigger in a
> > > >>>> task that has switched to the patching mm, could they use
> > > >>>> bpf_probe_write_user() - which uses probe_kernel_write() after
> > > >>>> checking that KERNEL_DS isn't active and that access_ok() passes - to
> > > >>>> overwrite kernel text that is mapped writable in the patching mm?
> > > >>>
> > > >>> Yes, this is a good point. I guess text_poke() should be defined with
> > > >>> “__kprobes” and open-code memcpy.
> > > >>>
> > > >>> Does it sound reasonable?
> > > >>
> > > >> Doesn't __text_poke() as implemented in the proposed patch use a
> > > >> couple other kernel functions, too? Like switch_mm_irqs_off() and
> > > >> pte_clear() (which can be a call into a separate function on paravirt
> > > >> kernels)?
> > > >
> > > > I will move the pte_clear() to be done after the poking mm was unloaded.
> > > > Give me a few minutes to send a sketch of what I think should be done.
> > >
> > > Err.. You are right, I don’t see an easy way of preventing a kprobe from
> > > being set on switch_mm_irqs_off(), and open-coding this monster is too ugly.
> > >
> > > The reasonable solution seems to me as taking all the relevant pieces of
> > > code (and data) that might be used during text-poking and encapsulating them, so they
> > > will be set in a memory area which cannot be kprobe'd. This can also be
> > > useful to write-protect data structures of code that calls text_poke(),
> > > e.g., static-keys. It can also protect data on that stack that is used
> > > during text_poke() from being overwritten from another core.
> > >
> > > This solution is somewhat similar to Igor Stoppa’s idea of using “enclaves”
> > > when doing write-rarely operations.
> > >
> > > Right now, I think that text_poke() will keep being susceptible to such
> > > an attack, unless you have a better suggestion.
> >
> > A relatively simple approach might be to teach BPF not to run kprobe
> > programs and such in contexts where current->mm isn't the active mm?
> > Maybe using nmi_uaccess_okay(), or something like that? It looks like
> > perf_callchain_user() also already uses that. Except that a lot of
> > this code is x86-specific...
>
> This sounds like exactly the right solution. If you're running from
> some unknown context (like NMI or tracing), then you should check
> nmi_uaccess_okay(). I think we should just promote that to be a
> non-arch-specific function (that returns true by default) and check it
> the relevant bpf_probe_xyz() functions.
This treat may also need for my work, like probe_user_read() we should
fail if nmi_uaccess_okay().
Thank you,
>
> Alexei, does that seem reasonable?
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH] net: dsa: read mac address from DT for slave device
From: xiaofeis @ 2019-02-25 13:28 UTC (permalink / raw)
To: Florian Fainelli
Cc: Vinod Koul, David S. Miller, linux-arm-msm, Bjorn Andersson,
Andrew Lunn, Vivien Didelot, Niklas Cassel, netdev
In-Reply-To: <9baf097f-37ea-2bef-92dd-25a7040a16ed@gmail.com>
Hi Florian
We have two slave DSA interfaces, wan0 and lan0, one is for wan port,
and the other is for lan port. Customer has it's mac address pool, they
want
to assign the mac address from the pool on wan0, lan0, and other
interfaces like
wifi, bt. Coreboot/uboot will populate it to the DTS node, so the driver
can
get it from it's node. For DSA slave interface, it already has it's own
DTS node, it's
easy to just add one porperty "local-mac-address" there for the usage in
DSA driver.
If not use DSA framework, normally we will use eth0.x and eth0.y for wan
and lan.
On this case, customer usually also assign the MAC address on these
logical interface
from it's pool.
On 2019-02-22 23:43, Florian Fainelli wrote:
> On 2/22/19 4:58 AM, Vinod Koul wrote:
>> From: Xiaofei Shen <xiaofeis@codeaurora.org>
>>
>> Before creating a slave netdevice, get the mac address from DTS and
>> apply in case it is valid.
>
> Can you explain your use case in details?
>
> Assigning a MAC address to a network device that represents a switch
> port does not quite make sense in general. The switch port is really
> representing one end of a pipe, so one side you have stations and on
> the
> other side, you have the CPU/management Ethernet MAC controller's MAC
> address which constitutes a station as well. The DSA slave network
> devices are just software constructs meant to steer traffic towards
> specific ports of the switch, but they are all from the perpsective of
> traffic reaching the CPU Port in the first place, therefore traffic
> that
> is generally a known unicast Ethernet frame with the CPU's MAC address
> as MAC DA (and of course all types of unknown MC, management traffic
> etc.)
>
> By default, DSA switch need to come up in a configuration where all
> ports (except CPU/management) must be strictly separate from every
> other
> port such that we can achieve what a standalone Ethernet NIC would do.
> This works because all ports are isolated from one another, so there is
> no cross talk and so having the same MAC address (the one from the CPU)
> on the DSA slave network devices just works, each port is a separate
> broadcast domain.
>
> Once you start bridging one or ore ports, the bridge root port will
> have
> a MAC address, most likely the one the CPU/management Ethernet MAC, but
> similarly, this is not an issue and that's exactly how a software
> bridge
> would work as well.
>
>>
>> Signed-off-by: Xiaofei Shen <xiaofeis@codeaurora.org>
>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>> ---
>> include/net/dsa.h | 1 +
>> net/dsa/dsa2.c | 1 +
>> net/dsa/slave.c | 5 ++++-
>> 3 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index b3eefe8e18fd..aa24ce756679 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -198,6 +198,7 @@ struct dsa_port {
>> unsigned int index;
>> const char *name;
>> const struct dsa_port *cpu_dp;
>> + const char *mac;
>> struct device_node *dn;
>> unsigned int ageing_time;
>> u8 stp_state;
>> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
>> index a1917025e155..afb7d9fa42f6 100644
>> --- a/net/dsa/dsa2.c
>> +++ b/net/dsa/dsa2.c
>> @@ -261,6 +261,7 @@ static int dsa_port_setup(struct dsa_port *dp)
>> int err = 0;
>>
>> memset(&dp->devlink_port, 0, sizeof(dp->devlink_port));
>> + dp->mac = of_get_mac_address(dp->dn);
>>
>> if (dp->type != DSA_PORT_TYPE_UNUSED)
>> err = devlink_port_register(ds->devlink, &dp->devlink_port,
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index a3fcc1d01615..8e64c4e947c6 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -1308,7 +1308,10 @@ int dsa_slave_create(struct dsa_port *port)
>> slave_dev->features = master->vlan_features | NETIF_F_HW_TC;
>> slave_dev->hw_features |= NETIF_F_HW_TC;
>> slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
>> - eth_hw_addr_inherit(slave_dev, master);
>> + if (port->mac && is_valid_ether_addr(port->mac))
>> + ether_addr_copy(slave_dev->dev_addr, port->mac);
>> + else
>> + eth_hw_addr_inherit(slave_dev, master);
>> slave_dev->priv_flags |= IFF_NO_QUEUE;
>> slave_dev->netdev_ops = &dsa_slave_netdev_ops;
>> slave_dev->switchdev_ops = &dsa_slave_switchdev_ops;
>>
^ permalink raw reply
* Re: [PATCH] net: dsa: Inherit dev addr from master
From: Andrew Lunn @ 2019-02-25 13:21 UTC (permalink / raw)
To: xiaofeis
Cc: Vinod Koul, David S. Miller, linux-arm-msm, Bjorn Andersson,
Vivien Didelot, Florian Fainelli, Niklas Cassel, netdev
In-Reply-To: <441ae81a814498876e4cfed6fd5b7bcb@codeaurora.org>
> I think we can remove the first inherit, but if keep it, we can see
> consistent address output by ifconfig -a before open master and slave
> interface.
True
But it also means we see inconsistent MAC addresses on the master. The
MAC address changes on open.
Maybe it would be better to change your master so it sets its MAC
address during probe? Everything is then consistent all the time.
Andrew
^ permalink raw reply
* Re: [PATCH net] sctp: get sctphdr by offset in sctp_compute_cksum
From: Xin Long @ 2019-02-25 13:20 UTC (permalink / raw)
To: Neil Horman; +Cc: LKML, network dev, linux-sctp, davem, Marcelo Ricardo Leitner
In-Reply-To: <20190225124614.GB30699@hmswarspite.think-freely.org>
On Mon, Feb 25, 2019 at 8:47 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Mon, Feb 25, 2019 at 07:25:37PM +0800, Xin Long wrote:
> > sctp_hdr(skb) only works when skb->transport_header is set properly.
> >
> > But in the path of nf_conntrack_in: sctp_packet() -> sctp_error()
> >
> > skb->transport_header is not guaranteed to be right value for sctp.
> > It will cause to fail to check the checksum for sctp packets.
> >
> > So fix it by using offset, which is always right in all places.
> >
> > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > Reported-by: Li Shuang <shuali@redhat.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> > include/net/sctp/checksum.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
> > index 32ee65a..1c6e6c0 100644
> > --- a/include/net/sctp/checksum.h
> > +++ b/include/net/sctp/checksum.h
> > @@ -61,7 +61,7 @@ static inline __wsum sctp_csum_combine(__wsum csum, __wsum csum2,
> > static inline __le32 sctp_compute_cksum(const struct sk_buff *skb,
> > unsigned int offset)
> > {
> > - struct sctphdr *sh = sctp_hdr(skb);
> > + struct sctphdr *sh = (struct sctphdr *)(skb->data + offset);
> > const struct skb_checksum_ops ops = {
> > .update = sctp_csum_update,
> > .combine = sctp_csum_combine,
> > --
> > 2.1.0
> >
> >
> Shouldn't you use skb_set_transport_header and skb_transport_header here?
you mean:
skb_set_transport_header(skb, offset);
sh = sctp_hdr(skb);
?
There's no place counting on here to set transport_header.
It will be a kinda redundant job, yet skb is 'const'.
^ permalink raw reply
* Re: [PATCH] net: dsa: Inherit dev addr from master
From: xiaofeis @ 2019-02-25 13:13 UTC (permalink / raw)
To: Andrew Lunn
Cc: Vinod Koul, David S. Miller, linux-arm-msm, Bjorn Andersson,
Vivien Didelot, Florian Fainelli, Niklas Cassel, netdev
In-Reply-To: <20190222143003.GL5653@lunn.ch>
On 2019-02-22 22:30, Andrew Lunn wrote:
> On Fri, Feb 22, 2019 at 06:26:54PM +0530, Vinod Koul wrote:
>> From: Xiaofei Shen <xiaofeis@codeaurora.org>
>>
>> When we create slave netdevice, the dev addr is inherited from master
>> but the master dev addr maybe NULL at that time, so inherit it again
>> while opening the slave.
>
> Hi Xiaofei, Vinod
>
> Inheriting it twice seems a bit clumsy. Can the first inherit be
> removed? Can you change your MAC driver to set its MAC address in
> probe?
>
> Andrew
Hi Andrew
Some mac driver set its MAC address in probe while some MAC driver do it
in the open function in the kernel.
This patch is checking whether the address is valid firstly, if it
already inherits a valid mac address, it will not inherit again.
I think we can remove the first inherit, but if keep it, we can see
consistent address output by ifconfig -a before open master and slave
interface.
Xiaofeis
^ permalink raw reply
* Re: [PATCH net v2 2/2] selftests: pmtu: add explicit tests for PMTU exceptions cleanup
From: Stefano Brivio @ 2019-02-25 12:56 UTC (permalink / raw)
To: Sabrina Dubroca, Paolo Abeni; +Cc: netdev, David S. Miller, David Ahern
In-Reply-To: <20190225123330.GA10994@bistromath.localdomain>
On Mon, 25 Feb 2019 13:33:30 +0100
Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2019-02-25, 12:13:46 +0100, Paolo Abeni wrote:
> > + if ! timeout 1 ${ns_a} ip link del dev veth_A-R1; then
>
> That doesn't work. "ip link del" is stuck in a way that timeout can't
> terminate it, so this is still going to hang. Did you actually test
> this? :/
Indeed, upon actual testing, this hangs and the error is not reported.
> > + err " can't delete veth device in a timely
> > manner, PMTU dst likely leaked"
> > + return 1
> > + fi
> > + return 0
> > +}
You can use a subshell here, something like:
${ns_a} ip link del dev veth_A-R1 &
iplink_pid=$!
sleep 1
if [ "$(cat /proc/${iplink_pid}/cmdline 2>/dev/null | tr -d '\0')" = "iplinkdeldevveth_A-R1" ]; then
err " can't delete veth device in a timely manner, PMTU dst likely leaked"
return 1
fi
}
and now that I tried to run as a single test and couldn't find it, I
think it would also be worth it to rename the tests to something more
sensible.
Right now you have test_pmtu_ipv6_exception_cleanup() calling
test_cleanup_...(), I think it's confusing. Just call the test
test_cleanup_ipv6_exception(), it doesn't have so much to do with PMTU
anyway.
--
Stefano
^ permalink raw reply
* Re: [PATCH net] sctp: get sctphdr by offset in sctp_compute_cksum
From: Neil Horman @ 2019-02-25 12:46 UTC (permalink / raw)
To: Xin Long
Cc: linux-kernel, network dev, linux-sctp, davem,
Marcelo Ricardo Leitner
In-Reply-To: <2aee76766861ac9e46b7acf123d5d6e8f2adf8e0.1551093937.git.lucien.xin@gmail.com>
On Mon, Feb 25, 2019 at 07:25:37PM +0800, Xin Long wrote:
> sctp_hdr(skb) only works when skb->transport_header is set properly.
>
> But in the path of nf_conntrack_in: sctp_packet() -> sctp_error()
>
> skb->transport_header is not guaranteed to be right value for sctp.
> It will cause to fail to check the checksum for sctp packets.
>
> So fix it by using offset, which is always right in all places.
>
> Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> Reported-by: Li Shuang <shuali@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> include/net/sctp/checksum.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
> index 32ee65a..1c6e6c0 100644
> --- a/include/net/sctp/checksum.h
> +++ b/include/net/sctp/checksum.h
> @@ -61,7 +61,7 @@ static inline __wsum sctp_csum_combine(__wsum csum, __wsum csum2,
> static inline __le32 sctp_compute_cksum(const struct sk_buff *skb,
> unsigned int offset)
> {
> - struct sctphdr *sh = sctp_hdr(skb);
> + struct sctphdr *sh = (struct sctphdr *)(skb->data + offset);
> const struct skb_checksum_ops ops = {
> .update = sctp_csum_update,
> .combine = sctp_csum_combine,
> --
> 2.1.0
>
>
Shouldn't you use skb_set_transport_header and skb_transport_header here?
Neil
^ 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