netdev - Re: [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer

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: <CAFSKS=PZ5_FgtOmSq=9xCgYPmqnmwjp6v7JFwx1tyBCP8ayopQ@mail.gmail.com>
Date:   Thu, 14 Jan 2021 09:03:53 -0600
From:   George McCollister <george.mccollister@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Rob Herring <robh@...nel.org>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        "open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>
Subject: Re: [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer

On Wed, Jan 13, 2021 at 7:05 PM Vladimir Oltean <olteanv@...il.com> wrote:
> > +++ b/net/dsa/tag_xrs700x.c
> > @@ -0,0 +1,67 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * XRS700x tag format handling
> > + * Copyright (c) 2008-2009 Marvell Semiconductor
>
> Why does Marvell get copyright?

What Andrew said. I started with tag_trailer.c since it is quite
similar and it seemed wrong to remove the copyright.

>
> > + * Copyright (c) 2020 NovaTech LLC
> > + */
> > +
> > +#include <linux/etherdevice.h>
> > +#include <linux/list.h>
> > +#include <linux/slab.h>
>
> These 3 includes are not needed. You can probably remove them later
> though, if there is no other reason to resend.

Removed.

>
> > +#include <linux/bitops.h>
> > +
> > +#include "dsa_priv.h"
> > +
> > +static struct sk_buff *xrs700x_xmit(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +     struct dsa_port *dp = dsa_slave_to_port(dev);
> > +     u8 *trailer;
> > +
> > +     trailer = skb_put(skb, 1);
> > +     trailer[0] = BIT(dp->index);
> > +
> > +     return skb;
> > +}
> > +
> > +static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev,
> > +                                struct packet_type *pt)
> > +{
> > +     int source_port;
> > +     u8 *trailer;
> > +
> > +     if (skb_linearize(skb))
> > +             return NULL;
>
> We've been through this, there should be no reason to linearize an skb
> for a one-byte tail tag..

Sorry about this. You and Andrew started discussing it and I guess I
got distracted fixing the other issues. Removed. I'll retest after
making other changes to patches in the series but based on what you've
said it should be fine without it.

>
> > +
> > +     trailer = skb_tail_pointer(skb) - 1;
> > +
> > +     source_port = ffs((int)trailer[0]) - 1;
> > +
> > +     if (source_port < 0)
> > +             return NULL;
> > +
> > +     skb->dev = dsa_master_find_slave(dev, 0, source_port);
> > +     if (!skb->dev)
> > +             return NULL;
> > +
> > +     if (pskb_trim_rcsum(skb, skb->len - 1))
> > +             return NULL;
> > +
> > +     /* Frame is forwarded by hardware, don't forward in software. */
> > +     skb->offload_fwd_mark = 1;
> > +
> > +     return skb;
> > +}