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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CANn89iL-GyiBd80UOn5-sqeCHXkbVJ=vXTk91c7vAmTsO6M8iA@mail.gmail.com>
Date: Mon, 9 Dec 2024 10:53:59 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Ido Schimmel <idosch@...sch.org>
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 Sun, Dec 8, 2024 at 6:57 PM Ido Schimmel <idosch@...sch.org> wrote:
>
> 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?

None, will fix in v2.

>
> >       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);
> +

Good idea, will be included in v2, thanks !