netdev - Re: [PATCH net-next 4/6] net: dsa: sja1105: support flow-based redirection via virtual links

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: <CA+h21hpJCN6ud6bvJpF-P7by0upDVEqZBZbyzb3-wCVJkiWs=w@mail.gmail.com>
Date:   Mon, 4 May 2020 22:40:40 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Vivien Didelot <vivien.didelot@...il.com>
Cc:     netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Po Liu <po.liu@....com>,
        Xiaoliang Yang <xiaoliang.yang@....com>,
        Mingkai Hu <mingkai.hu@....com>,
        Christian Herber <christian.herber@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        vlad@...lov.dev, Jiri Pirko <jiri@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next 4/6] net: dsa: sja1105: support flow-based
 redirection via virtual links

On Mon, 4 May 2020 at 22:24, Vivien Didelot <vivien.didelot@...il.com> wrote:
>
> Hi Vladimir,
>
> On Mon, 4 May 2020 21:38:26 +0300, Vladimir Oltean <olteanv@...il.com> wrote:
> > Hi Vivien,
> >
> > On Mon, 4 May 2020 at 21:23, Vivien Didelot <vivien.didelot@...il.com> wrote:
> > >
> > > On Mon, 4 May 2020 14:19:13 -0400, Vivien Didelot <vivien.didelot@...il.com> wrote:
> > > > Hi Vladimir,
> > > >
> > > > On Mon,  4 May 2020 00:10:33 +0300, Vladimir Oltean <olteanv@...il.com> wrote:
> > > > > +           case FLOW_ACTION_REDIRECT: {
> > > > > +                   struct dsa_port *to_dp;
> > > > > +
> > > > > +                   if (!dsa_slave_dev_check(act->dev)) {
> > > > > +                           NL_SET_ERR_MSG_MOD(extack,
> > > > > +                                              "Destination not a switch port");
> > > > > +                           return -EOPNOTSUPP;
> > > > > +                   }
> > > > > +
> > > > > +                   to_dp = dsa_slave_to_port(act->dev);
> > > >
> > > > Instead of exporting two DSA core internal functions, I would rather expose
> > > > a new helper for drivers, such as this one:
> > > >
> > > >     struct dsa_port *dsa_dev_to_port(struct net_device *dev)
> > > >     {
> > > >         if (!dsa_slave_dev_check(dev))
> > > >             return -EOPNOTSUPP;
> > >
> > > Oops, NULL, not an integer error code, but you get the idea of public helpers.
> > >
> > > >
> > > >         return dsa_slave_to_port(dev);
> > > >     }
> > > >
> > > > The naming might not be the best, this helper could even be mirroring-specific,
> > > > I didn't really check the requirements for this functionality yet.
> > > >
> > > >
> > > > Thank you,
> > > >
> > > >       Vivien
> >
> > How about
> >
> > int dsa_slave_get_port_index(struct net_device *dev)
> > {
> >     if (!dsa_slave_dev_check(dev))
> >         return -EINVAL;
> >
> >     return dsa_slave_to_port(dev)->index;
> > }
> > EXPORT_SYMBOL_GPL(dsa_slave_get_port_index);
> >
> > also, where to put it? slave.c I suppose?
>
> dsa.c is the place for private implementation of public functions. "slave"
> is a core term, no need to expose it. Public helpers exposed in dsa.h usually
> scope the dsa_switch structure and an optional port index. mv88e6xxx allows
> mirroring an external device port,

For mirroring an entire port (via tc-matchall), the tc structures are
already parsed by DSA core and a simple API is given to drivers. The
discussion we're having is for flow-based mirroring (via tc-flower)
where that is not the case.

> so dsa_port would be preferred, but this
> can wait. So I'm thinking about implementing the following:
>
> net/dsa/dsa.c:
>
> int dsa_to_port_index(struct dsa_switch *ds, struct net_device *dev)

But let's assume for a second that mv88e6xxx supports flow-based
mirroring/redirection too.
Aren't we limiting ourselves uselessly here, by requiring the caller
to pass a ds pointer just to perform validation on it? I think it's a
valid use case to want to support cross-chip mirroring/redirection
sometime in the future. Both sja1105 and mv88e6xxx support that kind
of setup, you just need to set the destination port to
dsa_towards_port() in case the dp->ds found by dsa_slave_to_port does
not coincide with ours. But surprise, using the syntactic sugar API
we're introducing here, we'd get -EINVAL and we would have to somehow
try again and guess with a ds pointer we don't have.

> {
>     struct dsa_port *dp;
>
>     if (!dsa_slave_dev_check(dev))
>         return -ENODEV;
>
>     dp = dsa_slave_to_port(dev);
>
>     if (dp->ds != ds)
>         return -EINVAL;
>
>     return dp->index;
> }
>
> include/net/dsa.h:
>
> int dsa_to_port_index(struct dsa_switch *ds, struct net_device *dev);
>
>
> What do you think?

I'm actually not convinced about this idea. I think the function that
should be called should be named dsa_slave_to_port, and it should
return a struct dsa_port. Quite conveniently, that function already
exists. I'm not actually sure what are the issues of exposing the
existing functions.

>
>         Vivien

Thanks,
-Vladimir