netdev - Re: [PATCH bpf-next] netkit: Add some ethtool ops to provide information to user

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]

Date: Wed, 6 Dec 2023 11:57:11 +0100
From: Daniel Borkmann <daniel@...earbox.net>
To: Feng Zhou <zhoufeng.zf@...edance.com>,
 Nikolay Aleksandrov <razor@...ckwall.org>, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, yangzhenze@...edance.com,
 wangdongdong.6@...edance.com, tangchen.1@...edance.com
Subject: Re: [PATCH bpf-next] netkit: Add some ethtool ops to provide
 information to user

On 12/6/23 7:59 AM, Feng Zhou wrote:
> 在 2023/12/4 23:22, Daniel Borkmann 写道:
>> Thanks, so the netkit_get_link_ksettings is optional. 
> 
> Yes, netkit_get_link_ksettings really not necessary, I just added it in line with veth.
> 
> I don't quite
>> follow what you
>> mean with regards to your business logic in veth to obtain peer ifindex. What does
>> the script do exactly with the peer ifindex (aka /why/ is it needed), could you
>> elaborate some more - it's still somewhat too vague? 🙂 E.g. why it does not suffice
>> to look at the device type or other kind of attributes?
> 
> The scripting logic of the business colleagues should just be simple logging records, using ethtool. Then they saw that netkit has this missing, so raised this requirement, so I sent this patch, wanting to hear your opinions. If you don't think it's necessary, let the business colleagues modify it.

So it is basically only logging the peer_ifindex data from ethtool but nothing
more is done with it? Meaning, this was raised as a requirement because this was
missing from the logging data, or was there anything more to it?

The peer ifindex is already available via IFLA_LINK (which does dev_get_iflink()
internally to fetch the peer's ifindex). This is also what you see in iproute2:

# ip l
[...]
163: nk0@nk1: <BROADCAST,MULTICAST,NOARP,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
164: nk1@nk0: <BROADCAST,MULTICAST,NOARP,M-DOWN> mtu 1500 qdisc noop state DOWN group default qlen 1000
     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
[...]
# ip netns add blue
# ip link set nk1 netns blue
# ip l
[...]
163: nk0@...64: <BROADCAST,MULTICAST,NOARP> mtu 1500 qdisc noop state DOWN group default qlen 1000
     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff link-netns blue
[...]

The `if164` denotes the peer's ifindex and `link-netns blue` provides info on the netns of the peer.
This is much better and more generic than the ethtool hack. I would suggest changing the logic to
rather look at data populated by rtnl_fill_link_netnsid() instead.

Thanks,
Daniel