Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Dec 04, 2023 at 10:17:05AM -0800, Jakub Kicinski wrote:
> On Mon, 4 Dec 2023 13:00:35 +0200 Vladimir Oltean wrote:
> > If I may intervene. The "request state" will ultimately go away once all
> > drivers are converted. I know it's more fragile and not all fields are
> > valid, but I think I would like drivers to store the kernel_ variant of
> > the structure, because more stuff will be added to the kernel_ variant
> > in the future (the hwtstamp provider + qualifier), and doing this from
> > the beginning will avoid reworking them again.
> 
> Okay, you know the direction of this work better, so:
> 
> pw-bot: under-review

I mean your observation is in principle fair. If drivers save the struct
kernel_hwtstamp_config in the set() method and give it back in the get()
method (this is very widespread BTW), it's reasonable to question what
happens with the temporary fields, ifr and copied_to_user. Won't we
corrupt the teporary fields of the kernel_hwtstamp_config structure from
the set() with the previous ones from the get()?

The answer, I think, is that we do, but in a safe way. Because we implement
ndo_hwtstamp_set(), the copied_to_user that we save is false (aka "the
driver implementation didn't call copy_to_user()"). And when we give
this structure back in ndo_hwtstamp_get(), we overwrite false with false,
and a good ifr pointer with a bad one.

But the only reason we transport the ifr along with the
kernel_hwtstamp_config is for generic_hwtstamp_ioctl_lower() to work,
aka a new API upper driver on top of an old API real driver. Which is
not the case here, and no one looks at the stale ifr pointer.

It's a lot to think about to make sure that something bad won't happen,
I agree. I still don't believe it will break in subtle ways, but nonetheless
I do recognize the tradeoff. One approach is more straightforward
code-wise but more subtle behavior-wise, and the other is the opposite.

> 
> Report-bugs-to: Vladimir Oltean <olteanv@gmail.com>
> 
> :P

Hmm, I'm not sure if I want to go that far. Alex is free to choose
whichever implementation he sees fit, and so, he is also responsible
for the end result, in spite of any review feedback received. Please
don't consider my message as anything more than a suggestion.