netdev - Re: [PATCH net-next RFC 10/12] net: dsa: Move FDB dump implementation inside DSA

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87wp7g5bkv.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
Date:   Mon, 10 Jul 2017 15:36:00 -0400
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Arkadi Sharshevsky <arkadis@...lanox.com>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, jiri@...nulli.us, ivecera@...hat.com,
        f.fainelli@...il.com, andrew@...n.ch, Woojung.Huh@...rochip.com,
        stephen@...workplumber.org, mlxsw@...lanox.com,
        Arkadi Sharshevsky <arkadis@...lanox.com>
Subject: Re: [PATCH net-next RFC 10/12] net: dsa: Move FDB dump implementation inside DSA

Hi Arkadi,

Arkadi Sharshevsky <arkadis@...lanox.com> writes:

> +struct dsa_slave_dump_ctx {
> +	struct net_device *dev;
> +	struct sk_buff *skb;
> +	struct netlink_callback *cb;
> +	int idx;
> +};
> +
>  struct dsa_switch_ops {
>  	/*
>  	 * Legacy probing.
> @@ -392,9 +399,7 @@ struct dsa_switch_ops {
>  	int	(*port_fdb_del)(struct dsa_switch *ds, int port,
>  				const unsigned char *addr, u16 vid);
>  	int	(*port_fdb_dump)(struct dsa_switch *ds, int port,
> -				 struct switchdev_obj_port_fdb *fdb,
> -				  switchdev_obj_dump_cb_t *cb);
> -
> +				 struct dsa_slave_dump_ctx *dump);

I would prefer to pass a callback to the driver, so that we can call
port_fdb_dump for other interfaces like debugfs, and for non exposed
switch ports (CPU and DSA links) as well. Something like:

    typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
                                  bool is_static, void *data);

    int (*port_fdb_dump)(struct dsa_switch *ds, int port,
                         dsa_fdb_dump_cb_t *cb, void *data);

Then dsa_slave_dump_ctx and dsa_slave_port_fdb_do_dump can be static to
net/dsa/slave.c.

> +int dsa_slave_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
> +			    struct net_device *dev,
> +			    struct net_device *filter_dev, int *idx)
> +{
> +	struct dsa_slave_dump_ctx dump = {
> +		.dev = dev,
> +		.skb = skb,
> +		.cb = cb,
> +		.idx = *idx,
> +	};
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_port *dp = p->dp;
> +	struct dsa_switch *ds = dp->ds;
> +	int err;
> +
> +	if (!ds->ops->port_fdb_dump) {
> +		err = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
> +out:
> +	*idx = dump.idx;
> +	return err;

There is no need to reassign *idx in case of error:

    if (!ds->ops->port_fdb_dump)
        return -EOPNOTSUPP;

    err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
    *idx = dump.idx;

    return err;

> +	.ndo_fdb_dump		= dsa_slave_port_fdb_dump,

And s/dsa_slave_port_fdb_dump/dsa_slave_fdb_dump/ here will be even
better ;-)


Thanks,

        Vivien