netdev - Re: [PATCH net-next RFC] Add NDOs for hardware timestamp get/set

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: <20230401181629.l2qkc47q2vjank3z@skbuf>
Date:   Sat, 1 Apr 2023 21:16:29 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Max Georgiev <glipus@...il.com>, kory.maincent@...tlin.com,
        netdev@...r.kernel.org, maxime.chevallier@...tlin.com
Subject: Re: [PATCH net-next RFC] Add NDOs for hardware timestamp get/set

On Fri, Mar 31, 2023 at 11:10:41AM -0700, Jakub Kicinski wrote:
> On Fri, 31 Mar 2023 11:51:06 -0600 Max Georgiev wrote:
> > On Thu, Mar 30, 2023 at 11:35 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > > I wonder if we should pass in
> > >
> > >         struct netlink_ext_ack *extack
> > >
> > > and maybe another structure for future extensions?
> > > So we don't have to change the drivers again when we extend uAPI.  
> > 
> > Would these two extra parameters be ignored by drivers in this initial
> > version of NDO hw timestamp API implementation?
> 
> Yup, and passed in as NULL.
> 
> See struct kernel_ethtool_coalesce for example of a kernel side
> structure extending a fixed-size uAPI struct ethtool_coalesce.
> 
> So we would add a struct kernel_hwtstamp_config which would be 
> empty for now, but we can make it not empty later.
> 
> Vladimir, does that sound reasonable or am I over-thinking?

So in principle I'm okay with the NULL extack (even though we could consider
doing something with the netlink message instead of letting it go to waste;
a suggestion may be to print the _msg to the kernel log, like store_bridge_parm()
does).

I missed the discussions around the creation of struct kernel_ethtool_coalesce,
but I imagine that ethtool_ops->set_coalesce() now takes both struct ethtool_coalesce,
as well as struct kernel_ethtool_coalesce, to avoid refactoring drivers even further
than just patching their function prototype? I don't think that argument would
apply here, for a completely new API?

I'm also okay, in principle, with having a struct kernel_hwtstamp_config, but
I have two problems with the way in which you've suggested it be implemented.
The first is not really only my problem, but rather, I don't think you
can have empty structures in C - I tried!

/* C doesn't allow empty structures, bah! */
struct sja1105_tas_data {
	u8 dummy;
};

The second is that I would actively dislike an ndo_hwtstamp_set() API
where half of the arguments are in one structure and half in the other.
I believe it's much easier, and cleaner, to make struct kernel_hwtstamp_config
duplicate the exact same fields from struct hwtstamp_config, and copy
those fields one by one from one structure to the other (to avoid issues
with UAPI field alignment mismatches). So we could pass only the extensible
kernel_hwtstamp_config to ndo_hwtstamp_set() and ndo_hwtstamp_get().