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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z1XeHQJkeJQ1OBFz@shredder>
Date: Sun, 8 Dec 2024 19:57:49 +0200
From: Ido Schimmel <idosch@...sch.org>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, Simon Horman <horms@...nel.org>,
	Roopa Prabhu <roopa@...dia.com>,
	Kuniyuki Iwashima <kuniyu@...zon.com>, eric.dumazet@...il.com
Subject: Re: [PATCH net-next 1/3] rtnetlink: add ndo_fdb_dump_context

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@...gle.com>

Reviewed-by: Ido Schimmel <idosch@...dia.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);