Re: [PATCH net-next 1/3] rtnetlink: add ndo_fdb_dump_context

[Ido Schimmel <idosch@idosch.org>]

On Sat, Dec 07, 2024 at 04:22:46PM +0000, Eric Dumazet wrote:
> rtnl_fdb_dump() and various ndo_fdb_dump() helpers share
> a hidden layout of cb->ctx.
> 
> Before switching rtnl_fdb_dump() to for_each_netdev_dump()
> in the following patch, make this more explicit.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

A couple of nits in case you have v2

> ---
>  .../ethernet/freescale/dpaa2/dpaa2-switch.c   |  3 ++-
>  drivers/net/ethernet/mscc/ocelot_net.c        |  3 ++-
>  drivers/net/vxlan/vxlan_core.c                |  5 ++--
>  include/linux/rtnetlink.h                     |  7 ++++++
>  net/bridge/br_fdb.c                           |  3 ++-
>  net/core/rtnetlink.c                          | 24 +++++++++----------
>  net/dsa/user.c                                |  3 ++-
>  7 files changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> index a293b08f36d46dfde7e25412951da78c15e2dfd6..de383e6c6d523f01f02cb3c3977b1c448a3ac9a7 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
> @@ -781,12 +781,13 @@ static int dpaa2_switch_fdb_dump_nl(struct fdb_dump_entry *entry,
>  				    struct ethsw_dump_ctx *dump)
>  {
>  	int is_dynamic = entry->type & DPSW_FDB_ENTRY_DINAMIC;
> +	struct ndo_fdb_dump_context *ctx = (void *)dump->cb->ctx;

Any reason not to maintain reverse xmas tree here?

>  	u32 portid = NETLINK_CB(dump->cb->skb).portid;
>  	u32 seq = dump->cb->nlh->nlmsg_seq;
>  	struct nlmsghdr *nlh;
>  	struct ndmsg *ndm;
>  
> -	if (dump->idx < dump->cb->args[2])
> +	if (dump->idx < ctx->fdb_idx)
>  		goto skip;
>  
>  	nlh = nlmsg_put(dump->skb, portid, seq, RTM_NEWNEIGH,

[...]

> diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
> index 14b88f55192085def8f318c7913a76d5447b4975..a91dfea64724615c9db778646e52cb8573f47e06 100644
> --- a/include/linux/rtnetlink.h
> +++ b/include/linux/rtnetlink.h
> @@ -178,6 +178,13 @@ void rtnetlink_init(void);
>  void __rtnl_unlock(void);
>  void rtnl_kfree_skbs(struct sk_buff *head, struct sk_buff *tail);
>  
> +/* Shared by rtnl_fdb_dump() and various ndo_fdb_dump() helpers. */
> +struct ndo_fdb_dump_context {
> +	unsigned long s_h;
> +	unsigned long s_idx;
> +	unsigned long fdb_idx;
> +};
> +
>  extern int ndo_dflt_fdb_dump(struct sk_buff *skb,
>  			     struct netlink_callback *cb,
>  			     struct net_device *dev,
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 82bac2426631bcea63ea834e72f074fa2eaf0cee..902694c0ce643ec448978e4c4625692ccb1facd9 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -955,6 +955,7 @@ int br_fdb_dump(struct sk_buff *skb,
>  		struct net_device *filter_dev,
>  		int *idx)
>  {
> +	struct ndo_fdb_dump_context *ctx = (void *)cb->ctx;
>  	struct net_bridge *br = netdev_priv(dev);
>  	struct net_bridge_fdb_entry *f;
>  	int err = 0;

Unlikely that the context will ever grow past 48 bytes, but might be
worthwhile to add:

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index cdedb46edc2f..8fe252c298a2 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4919,6 +4919,8 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	int fidx = 0;
 	int err;
 
+	NL_ASSERT_CTX_FITS(struct ndo_fdb_dump_context);
+
 	if (cb->strict_check)
 		err = valid_fdb_dump_strict(cb->nlh, &br_idx, &brport_idx,
 					    cb->extack);