netdev - Re: [PATCH v6 net-next] net-core: add InMacErrors counter

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: <20220202205916.58f4a592@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 2 Feb 2022 20:59:16 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jeffrey Ji <jeffreyjilinux@...il.com>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        "David S . Miller" <davem@...emloft.net>,
        Brian Vazquez <brianvv@...gle.com>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        jeffreyji <jeffreyji@...gle.com>
Subject: Re: [PATCH v6 net-next] net-core: add InMacErrors counter

On Tue,  1 Feb 2022 22:28:45 +0000 Jeffrey Ji wrote:
> From: jeffreyji <jeffreyji@...gle.com>
> 
> Increment InMacErrors counter when packet dropped due to incorrect dest
> MAC addr.
> 
> An example when this drop can occur is when manually crafting raw
> packets that will be consumed by a user space application via a tap
> device. For testing purposes local traffic was generated using trafgen
> for the client and netcat to start a server
> 
> example output from nstat:
> \~# nstat -a | grep InMac
> Ip6InMacErrors                  0                  0.0
> IpExtInMacErrors                1                  0.0

I had another thing and this still doesn't sit completely well 
with me :(

Shouldn't we count those drops as skb->dev->rx_dropped?
Commonly NICs will do such filtering and if I got it right
in struct rtnl_link_stats64 kdoc - report them as rx_dropped.
It'd be inconsistent if on a physical interface we count
these as rx_dropped and on SW interface (or with promisc enabled 
etc.) in the SNMP counters. 
Or we can add a new link stat that NICs can use as well.

In fact I'm not sure this is really a IP AKA L3 statistic,
it's the L2 address that doesn't match.

If everyone disagrees - should we at least rename the MIB counter
similarly to the drop reason? Experience shows that users call for 
help when they see counters with Error in their name, I'd vote for
IpExtInDropOtherhost or some such. The statistic should also be
documented in Documentation/networking/snmp_counter.rst