netdev - Re: [net-next v4 2/3] net: dsa: tag_yt921x: add support for Motorcomm YT921x tags

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: <e591888a-30c3-4482-9ef1-9f7060e85867@lunn.ch>
Date: Mon, 18 Aug 2025 21:54:44 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Yangfl <mmyangfl@...il.com>
Cc: netdev@...r.kernel.org, Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Simon Horman <horms@...nel.org>,
	Russell King <linux@...linux.org.uk>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [net-next v4 2/3] net: dsa: tag_yt921x: add support for
 Motorcomm YT921x tags

> > > +static struct sk_buff *
> > > +yt921x_tag_rcv(struct sk_buff *skb, struct net_device *netdev)
> > > +{
> > > +     unsigned int port;
> > > +     __be16 *tag;
> > > +     u16 rx;
> > > +
> > > +     if (unlikely(!pskb_may_pull(skb, YT921X_TAG_LEN)))
> > > +             return NULL;
> > > +
> > > +     tag = (__be16 *)skb->data;
> > > +
> > > +     /* Locate which port this is coming from */
> > > +     rx = ntohs(tag[1]);
> > > +     if (unlikely((rx & YT921X_TAG_PORT_EN) == 0)) {
> > > +             netdev_err(netdev, "Unexpected rx tag 0x%04x\n", rx);
> > > +             return NULL;
> > > +     }
> > > +
> > > +     port = FIELD_GET(YT921X_TAG_RX_PORT_M, rx);
> > > +     skb->dev = dsa_conduit_find_user(netdev, 0, port);
> > > +     if (unlikely(!skb->dev)) {
> > > +             netdev_err(netdev, "Cannot locate rx port %u\n", port);
> > > +             return NULL;
> > > +     }
> >
> > O.K. Stop. Think.
> >
> > You changed the rate limiting to an unlimiting netdev_err().
> >
> > What is the difference? Under what conditions would you want to use
> > rate limiting? When would you not use rate limiting?
> >
> > Please reply and explain why you made this change.
> >
> >         Andrew
> 
> I copied the limited version from tag_vsc73xx_8021q.
> 
> Under no conditions I expect either of them to appear: it is the case
> when I did my own tests; unless something really bad happens, like
> pouring a cup of coffee over your device.
> 
> I know rate limiting is a way to prevent flooding the same message
> over dmesg, but if an event never happens, I would consider two
> methods are exchangeable. Theoretically if an event never happens, no
> warnings would ever be needed, but I placed one here in case you
> destroy your device accidentally.
> 
> Thus if you think rate limiting is not appropriate here, I would fix
> it with another.

My thinking is, every single packet come through here. That could be
100,000 packets per second. We have no control over these packets,
they are from other hosts, they can contain anything, included
specially crafted packets, ping of death packets, etc. If there is a
way to trigger these prints, and they print 100,000 times per second,
it makes a good DoS. The box is completely useless. With rate
limiting, the box keeps working, there is no DoS, but you get to see
the error messages.

The basic networking philosophy is that anything per packet related
which could cause a print should be rate limited.

      Andrew