* [PATCH net v2 1/3] ethernet: ucc_geth: set dev->max_mtu to 1518
2020-12-18 10:55 [PATCH net v2 0/3] ucc_geth fixes Rasmus Villemoes
@ 2020-12-18 10:55 ` Rasmus Villemoes
2020-12-20 8:05 ` Vladimir Oltean
2020-12-18 10:55 ` [PATCH net v2 2/3] ethernet: ucc_geth: fix definition and size of ucc_geth_tx_global_pram Rasmus Villemoes
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Rasmus Villemoes @ 2020-12-18 10:55 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski
Cc: netdev, Zhao Qiang, Li Yang, Andrew Lunn, linux-kernel,
Rasmus Villemoes, Vladimir Oltean
All the buffers and registers are already set up appropriately for an
MTU slightly above 1500, so we just need to expose this to the
networking stack. AFAICT, there's no need to implement .ndo_change_mtu
when the receive buffers are always set up to support the max_mtu.
This fixes several warnings during boot on our mpc8309-board with an
embedded mv88e6250 switch:
mv88e6085 mdio@e0102120:10: nonfatal error -34 setting MTU 1500 on port 0
...
mv88e6085 mdio@e0102120:10: nonfatal error -34 setting MTU 1500 on port 4
ucc_geth e0102000.ethernet eth1: error -22 setting MTU to 1504 to include DSA overhead
The last line explains what the DSA stack tries to do: achieving an MTU
of 1500 on-the-wire requires that the master netdevice connected to
the CPU port supports an MTU of 1500+the tagging overhead.
Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
drivers/net/ethernet/freescale/ucc_geth.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 714b501be7d0..380c1f09adaf 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -3889,6 +3889,7 @@ static int ucc_geth_probe(struct platform_device* ofdev)
INIT_WORK(&ugeth->timeout_work, ucc_geth_timeout_work);
netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, 64);
dev->mtu = 1500;
+ dev->max_mtu = 1518;
ugeth->msg_enable = netif_msg_init(debug.msg_enable, UGETH_MSG_DEFAULT);
ugeth->phy_interface = phy_interface;
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net v2 1/3] ethernet: ucc_geth: set dev->max_mtu to 1518
2020-12-18 10:55 ` [PATCH net v2 1/3] ethernet: ucc_geth: set dev->max_mtu to 1518 Rasmus Villemoes
@ 2020-12-20 8:05 ` Vladimir Oltean
0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Oltean @ 2020-12-20 8:05 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: David S . Miller, Jakub Kicinski, netdev, Zhao Qiang, Li Yang,
Andrew Lunn, linux-kernel, Vladimir Oltean
On Fri, Dec 18, 2020 at 11:55:36AM +0100, Rasmus Villemoes wrote:
> All the buffers and registers are already set up appropriately for an
> MTU slightly above 1500, so we just need to expose this to the
> networking stack. AFAICT, there's no need to implement .ndo_change_mtu
> when the receive buffers are always set up to support the max_mtu.
>
> This fixes several warnings during boot on our mpc8309-board with an
> embedded mv88e6250 switch:
>
> mv88e6085 mdio@e0102120:10: nonfatal error -34 setting MTU 1500 on port 0
> ...
> mv88e6085 mdio@e0102120:10: nonfatal error -34 setting MTU 1500 on port 4
> ucc_geth e0102000.ethernet eth1: error -22 setting MTU to 1504 to include DSA overhead
>
> The last line explains what the DSA stack tries to do: achieving an MTU
> of 1500 on-the-wire requires that the master netdevice connected to
> the CPU port supports an MTU of 1500+the tagging overhead.
>
> Fixes: bfcb813203e6 ("net: dsa: configure the MTU for switch ports")
> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net v2 2/3] ethernet: ucc_geth: fix definition and size of ucc_geth_tx_global_pram
2020-12-18 10:55 [PATCH net v2 0/3] ucc_geth fixes Rasmus Villemoes
2020-12-18 10:55 ` [PATCH net v2 1/3] ethernet: ucc_geth: set dev->max_mtu to 1518 Rasmus Villemoes
@ 2020-12-18 10:55 ` Rasmus Villemoes
2020-12-18 10:55 ` [PATCH net v2 3/3] ethernet: ucc_geth: fix use-after-free in ucc_geth_remove() Rasmus Villemoes
2020-12-22 1:40 ` [PATCH net v2 0/3] ucc_geth fixes patchwork-bot+netdevbpf
3 siblings, 0 replies; 6+ messages in thread
From: Rasmus Villemoes @ 2020-12-18 10:55 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski
Cc: netdev, Zhao Qiang, Li Yang, Andrew Lunn, linux-kernel, Rasmus Villemoes
Table 8-53 in the QUICC Engine Reference manual shows definitions of
fields up to a size of 192 bytes, not just 128. But in table 8-111,
one does find the text
Base Address of the Global Transmitter Parameter RAM Page. [...]
The user needs to allocate 128 bytes for this page. The address must
be aligned to the page size.
I've checked both rev. 7 (11/2015) and rev. 9 (05/2018) of the manual;
they both have this inconsistency (and the table numbers are the
same).
Adding a bit of debug printing, on my board the struct
ucc_geth_tx_global_pram is allocated at offset 0x880, while
the (opaque) ucc_geth_thread_data_tx gets allocated immediately
afterwards, at 0x900. So whatever the engine writes into the thread
data overlaps with the tail of the global tx pram (and devmem says
that something does get written during a simple ping).
I haven't observed any failure that could be attributed to this, but
it seems to be the kind of thing that would be extremely hard to
debug. So extend the struct definition so that we do allocate 192
bytes.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
drivers/net/ethernet/freescale/ucc_geth.h | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.h b/drivers/net/ethernet/freescale/ucc_geth.h
index 3fe903972195..c80bed2c995c 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.h
+++ b/drivers/net/ethernet/freescale/ucc_geth.h
@@ -575,7 +575,14 @@ struct ucc_geth_tx_global_pram {
u32 vtagtable[0x8]; /* 8 4-byte VLAN tags */
u32 tqptr; /* a base pointer to the Tx Queues Memory
Region */
- u8 res2[0x80 - 0x74];
+ u8 res2[0x78 - 0x74];
+ u64 snums_en;
+ u32 l2l3baseptr; /* top byte consists of a few other bit fields */
+
+ u16 mtu[8];
+ u8 res3[0xa8 - 0x94];
+ u32 wrrtablebase; /* top byte is reserved */
+ u8 res4[0xc0 - 0xac];
} __packed;
/* structure representing Extended Filtering Global Parameters in PRAM */
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net v2 3/3] ethernet: ucc_geth: fix use-after-free in ucc_geth_remove()
2020-12-18 10:55 [PATCH net v2 0/3] ucc_geth fixes Rasmus Villemoes
2020-12-18 10:55 ` [PATCH net v2 1/3] ethernet: ucc_geth: set dev->max_mtu to 1518 Rasmus Villemoes
2020-12-18 10:55 ` [PATCH net v2 2/3] ethernet: ucc_geth: fix definition and size of ucc_geth_tx_global_pram Rasmus Villemoes
@ 2020-12-18 10:55 ` Rasmus Villemoes
2020-12-22 1:40 ` [PATCH net v2 0/3] ucc_geth fixes patchwork-bot+netdevbpf
3 siblings, 0 replies; 6+ messages in thread
From: Rasmus Villemoes @ 2020-12-18 10:55 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski
Cc: netdev, Zhao Qiang, Li Yang, Andrew Lunn, linux-kernel, Rasmus Villemoes
ugeth is the netdiv_priv() part of the netdevice. Accessing the memory
pointed to by ugeth (such as done by ucc_geth_memclean() and the two
of_node_puts) after free_netdev() is thus use-after-free.
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
drivers/net/ethernet/freescale/ucc_geth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 380c1f09adaf..3f9fca061cc0 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -3935,12 +3935,12 @@ static int ucc_geth_remove(struct platform_device* ofdev)
struct device_node *np = ofdev->dev.of_node;
unregister_netdev(dev);
- free_netdev(dev);
ucc_geth_memclean(ugeth);
if (of_phy_is_fixed_link(np))
of_phy_deregister_fixed_link(np);
of_node_put(ugeth->ug_info->tbi_node);
of_node_put(ugeth->ug_info->phy_node);
+ free_netdev(dev);
return 0;
}
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2 0/3] ucc_geth fixes
2020-12-18 10:55 [PATCH net v2 0/3] ucc_geth fixes Rasmus Villemoes
` (2 preceding siblings ...)
2020-12-18 10:55 ` [PATCH net v2 3/3] ethernet: ucc_geth: fix use-after-free in ucc_geth_remove() Rasmus Villemoes
@ 2020-12-22 1:40 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2020-12-22 1:40 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: davem, kuba, netdev, qiang.zhao, leoyang.li, andrew, linux-kernel
Hello:
This series was applied to netdev/net.git (refs/heads/master):
On Fri, 18 Dec 2020 11:55:35 +0100 you wrote:
> This is three bug fixes that fell out of a series of cleanups of the
> ucc_geth driver. Please consider applying via the net tree.
>
> v2: reorder and split off from larger series; add Andrew's R-b to
> patch 1; only move the free_netdev() call in patch 3.
>
> Rasmus Villemoes (3):
> ethernet: ucc_geth: set dev->max_mtu to 1518
> ethernet: ucc_geth: fix definition and size of ucc_geth_tx_global_pram
> ethernet: ucc_geth: fix use-after-free in ucc_geth_remove()
>
> [...]
Here is the summary with links:
- [net,v2,1/3] ethernet: ucc_geth: set dev->max_mtu to 1518
https://git.kernel.org/netdev/net/c/1385ae5c30f2
- [net,v2,2/3] ethernet: ucc_geth: fix definition and size of ucc_geth_tx_global_pram
https://git.kernel.org/netdev/net/c/887078de2a23
- [net,v2,3/3] ethernet: ucc_geth: fix use-after-free in ucc_geth_remove()
https://git.kernel.org/netdev/net/c/e925e0cd2a70
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread