RE: [EXT] [PATCH] net: marvell: prestera: fix handling IPv4 routes with nhid

From: Elad Nachman
Date: Sun Aug 06 2023 - 02:34:22 EST

Next message: John Watts: "Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes"
Previous message: Chen Linxuan: "Re: [PATCH v17 05/30] LoongArch: KVM: Add vcpu related header files"
In reply to: Jonas Gorski: "[PATCH] net: marvell: prestera: fix handling IPv4 routes with nhid"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Jonas Gorski <jonas.gorski@xxxxxxxx>
> Sent: Friday, August 4, 2023 1:12 PM
> To: Elad Nachman <enachman@xxxxxxxxxxx>; Taras Chornyi
> <taras.chornyi@xxxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric
> Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>;
> Paolo Abeni <pabeni@xxxxxxxxxx>; Yevhen Orlov
> <yevhen.orlov@xxxxxxxxxxx>; Oleksandr Mazur
> <oleksandr.mazur@xxxxxxxxxxx>
> Cc: Taras Chornyi [C] <tchornyi@xxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: [EXT] [PATCH] net: marvell: prestera: fix handling IPv4 routes with
> nhid
>
> External Email
>
> ----------------------------------------------------------------------
> Fix handling IPv4 routes referencing a nexthop via its id by replacing calls to
> fib_info_nh() with fib_info_nhc().
>
> Trying to add an IPv4 route referencing a nextop via nhid:
>
> $ ip link set up swp5
> $ ip a a 10.0.0.1/24 dev swp5
> $ ip nexthop add dev swp5 id 20 via 10.0.0.2
> $ ip route add 10.0.1.0/24 nhid 20
>
> triggers warnings when trying to handle the route:
>
> [ 528.805763] ------------[ cut here ]------------ [ 528.810437] WARNING: CPU: 3
> PID: 53 at include/net/nexthop.h:468 __prestera_fi_is_direct+0x2c/0x68
> [prestera] [ 528.820434] Modules linked in: prestera_pci act_gact act_police
> sch_ingress cls_u32 cls_flower prestera arm64_delta_tn48m_dn_led(O)
> arm64_delta_tn48m_dn_cpld(O) [last unloaded: prestera_pci]
> [ 528.837485] CPU: 3 PID: 53 Comm: kworker/u8:3 Tainted: G O
> 6.4.5 #1
> [ 528.845178] Hardware name: delta,tn48m-dn (DT) [ 528.849641]
> Workqueue: prestera_ordered __prestera_router_fib_event_work [prestera]
> [ 528.857352] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
> BTYPE=--) [ 528.864347] pc : __prestera_fi_is_direct+0x2c/0x68 [prestera] [
> 528.870135] lr : prestera_k_arb_fib_evt+0xb20/0xd50 [prestera] [
> 528.876007] sp : ffff80000b20bc90 [ 528.879336] x29: ffff80000b20bc90 x28:
> 0000000000000000 x27: ffff0001374d3a48 [ 528.886510] x26:
> ffff000105604000 x25: ffff000134af8a28 x24: ffff0001374d3800 [
> 528.893683] x23: ffff000101c89148 x22: ffff000101c89000 x21:
> ffff000101c89200 [ 528.900855] x20: ffff00013641fda0 x19:
> ffff800009d01088 x18: 0000000000000059 [ 528.908027] x17:
> 0000000000000277 x16: 0000000000000000 x15: 0000000000000000 [
> 528.915198] x14: 0000000000000003 x13: 00000000000fe400 x12:
> 0000000000000000 [ 528.922371] x11: 0000000000000002 x10:
> 0000000000000aa0 x9 : ffff8000013d2020 [ 528.929543] x8 :
> 0000000000000018 x7 : 000000007b1703f8 x6 : 000000001ca72f86 [
> 528.936715] x5 : 0000000033399ea7 x4 : 0000000000000000 x3 :
> ffff0001374d3acc [ 528.943886] x2 : 0000000000000000 x1 :
> ffff00010200de00 x0 : ffff000134ae3f80 [ 528.951058] Call trace:
> [ 528.953516] __prestera_fi_is_direct+0x2c/0x68 [prestera] [ 528.958952]
> __prestera_router_fib_event_work+0x100/0x158 [prestera] [ 528.965348]
> process_one_work+0x208/0x488 [ 528.969387] worker_thread+0x4c/0x430
> [ 528.973068] kthread+0x120/0x138 [ 528.976313]
> ret_from_fork+0x10/0x20 [ 528.979909] ---[ end trace 0000000000000000 ]--
> - [ 528.984998] ------------[ cut here ]------------ [ 528.989645] WARNING: CPU:
> 3 PID: 53 at include/net/nexthop.h:468 __prestera_fi_is_direct+0x2c/0x68
> [prestera] [ 528.999628] Modules linked in: prestera_pci act_gact act_police
> sch_ingress cls_u32 cls_flower prestera arm64_delta_tn48m_dn_led(O)
> arm64_delta_tn48m_dn_cpld(O) [last unloaded: prestera_pci]
> [ 529.016676] CPU: 3 PID: 53 Comm: kworker/u8:3 Tainted: G W O
> 6.4.5 #1
> [ 529.024368] Hardware name: delta,tn48m-dn (DT) [ 529.028830]
> Workqueue: prestera_ordered __prestera_router_fib_event_work [prestera]
> [ 529.036539] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
> BTYPE=--) [ 529.043533] pc : __prestera_fi_is_direct+0x2c/0x68 [prestera] [
> 529.049318] lr : __prestera_k_arb_fc_apply+0x280/0x2f8 [prestera] [
> 529.055452] sp : ffff80000b20bc60 [ 529.058781] x29: ffff80000b20bc60 x28:
> 0000000000000000 x27: ffff0001374d3a48 [ 529.065953] x26:
> ffff000105604000 x25: ffff000134af8a28 x24: ffff0001374d3800 [
> 529.073126] x23: ffff000101c89148 x22: ffff000101c89148 x21:
> ffff00013641fda0 [ 529.080299] x20: ffff000101c89000 x19: ffff000101c89020
> x18: 0000000000000059 [ 529.087471] x17: 0000000000000277 x16:
> 0000000000000000 x15: 0000000000000000 [ 529.094642] x14:
> 0000000000000003 x13: 00000000000fe400 x12: 0000000000000000 [
> 529.101814] x11: 0000000000000002 x10: 0000000000000aa0 x9 :
> ffff8000013cee80 [ 529.108985] x8 : 0000000000000018 x7 :
> 000000007b1703f8 x6 : 0000000000000018 [ 529.116157] x5 :
> 00000000d3497eb6 x4 : ffff000105604081 x3 : 000000008e979557 [
> 529.123329] x2 : 0000000000000000 x1 : ffff00010200de00 x0 :
> ffff000134ae3f80 [ 529.130501] Call trace:
> [ 529.132958] __prestera_fi_is_direct+0x2c/0x68 [prestera] [ 529.138394]
> prestera_k_arb_fib_evt+0x6b8/0xd50 [prestera] [ 529.143918]
> __prestera_router_fib_event_work+0x100/0x158 [prestera] [ 529.150313]
> process_one_work+0x208/0x488 [ 529.154348] worker_thread+0x4c/0x430
> [ 529.158030] kthread+0x120/0x138 [ 529.161274]
> ret_from_fork+0x10/0x20 [ 529.164867] ---[ end trace 0000000000000000 ]--
> -
>
> and results in a non offloaded route:
>
> $ ip route
> 10.0.0.0/24 dev swp5 proto kernel scope link src 10.0.0.1 rt_trap
> 10.0.1.0/24 nhid 20 via 10.0.0.2 dev swp5 rt_trap
>
> When creating a route referencing a nexthop via its ID, the nexthop will be
> stored in a separate nh pointer instead of the array of nexthops in the
> fib_info struct. This causes issues since fib_info_nh() only handles the
> nexthops array, but not the separate nh pointer, and will loudly WARN about
> it.
>
> In contrast fib_info_nhc() handles both, but returns a fib_nh_common
> pointer instead of a fib_nh pointer. Luckily we only ever access fields from
> the fib_nh_common parts, so we can just replace all instances of
> fib_info_nh() with fib_info_nhc() and access the fields via their
> fib_nh_common names.
>
> This allows handling IPv4 routes with an external nexthop, and they now get
> offloaded as expected:
>
> $ ip route
> 10.0.0.0/24 dev swp5 proto kernel scope link src 10.0.0.1 rt_trap
> 10.0.1.0/24 nhid 20 via 10.0.0.2 dev swp5 offload rt_offload
>
> Fixes: 396b80cb5cc8 ("net: marvell: prestera: Add neighbour cache
> accounting")
> Signed-off-by: Jonas Gorski <jonas.gorski@xxxxxxxx>
> ---
> .../ethernet/marvell/prestera/prestera_router.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_router.c
> b/drivers/net/ethernet/marvell/prestera/prestera_router.c
> index a9a1028cb17b..de317179a7dc 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_router.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_router.c
> @@ -166,11 +166,11 @@ prestera_util_neigh2nc_key(struct prestera_switch
> *sw, struct neighbour *n,
>
> static bool __prestera_fi_is_direct(struct fib_info *fi) {
> - struct fib_nh *fib_nh;
> + struct fib_nh_common *fib_nhc;
>
> if (fib_info_num_path(fi) == 1) {
> - fib_nh = fib_info_nh(fi, 0);
> - if (fib_nh->fib_nh_gw_family == AF_UNSPEC)
> + fib_nhc = fib_info_nhc(fi, 0);
> + if (fib_nhc->nhc_gw_family == AF_UNSPEC)
> return true;
> }
>
> @@ -261,7 +261,7 @@ static bool
> __prestera_util_kern_n_is_reachable_v4(u32 tb_id, __be32 *addr,
> struct net_device *dev)
> {
> - struct fib_nh *fib_nh;
> + struct fib_nh_common *fib_nhc;
> struct fib_result res;
> bool reachable;
>
> @@ -269,8 +269,8 @@ __prestera_util_kern_n_is_reachable_v4(u32 tb_id,
> __be32 *addr,
>
> if (!prestera_util_kern_get_route(&res, tb_id, addr))
> if (prestera_fi_is_direct(res.fi)) {
> - fib_nh = fib_info_nh(res.fi, 0);
> - if (dev == fib_nh->fib_nh_dev)
> + fib_nhc = fib_info_nhc(res.fi, 0);
> + if (dev == fib_nhc->nhc_dev)
> reachable = true;
> }
>
> @@ -324,7 +324,7 @@ prestera_kern_fib_info_nhc(struct fib_notifier_info
> *info, int n)
> if (info->family == AF_INET) {
> fen4_info = container_of(info, struct fib_entry_notifier_info,
> info);
> - return &fib_info_nh(fen4_info->fi, n)->nh_common;
> + return fib_info_nhc(fen4_info->fi, n);
> } else if (info->family == AF_INET6) {
> fen6_info = container_of(info, struct
> fib6_entry_notifier_info,
> info);
> --
> 2.41.0
>
>
> --
> BISDN GmbH
> Körnerstraße 7-10
> 10785 Berlin
> Germany
>
>
> Phone:
> +49-30-6108-1-6100
>
>
> Managing Directors:
> Dr.-Ing. Hagen Woesner, Andreas
> Köpsel
>
>
> Commercial register:
> Amtsgericht Berlin-Charlottenburg HRB 141569
> B
> VAT ID No: DE283257294

Acked-by: Elad Nachman <enachman@xxxxxxxxxxx>

Next message: John Watts: "Re: [PATCH v2 2/4] riscv: dts: allwinner: d1: Add CAN controller nodes"
Previous message: Chen Linxuan: "Re: [PATCH v17 05/30] LoongArch: KVM: Add vcpu related header files"
In reply to: Jonas Gorski: "[PATCH] net: marvell: prestera: fix handling IPv4 routes with nhid"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]