netdev - Re: [PATCH net-next v3 3/3] ptp: support event queue reader channel masks

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: Sat, 30 Sep 2023 10:01:55 +0200
From: Xabier Marquiegui <reibax@...il.com>
To: vinicius.gomes@...el.com
Cc: alex.maftei@....com,
	chrony-dev@...ony.tuxfamily.org,
	davem@...emloft.net,
	horms@...nel.org,
	mlichvar@...hat.com,
	netdev@...r.kernel.org,
	ntp-lists@...tcorallo.com,
	reibax@...il.com,
	richardcochran@...il.com,
	rrameshbabu@...dia.com,
	shuah@...nel.org
Subject: Re: [PATCH net-next v3 3/3] ptp: support event queue reader channel masks

Vinicius Costa Gomes <vinicius.gomes@...el.com> writes:

> Looking below, at the usability of the API, it feels too complicated, I
> was trying to think, "how an application would change the mask for
> itself": first it would need to know the PID of the process that created
> the fd, then it would have to find the OID associated with that PID, and
> then build the request.
> 
> And it has the problem of being error prone, for example, it's easy for
> an application to override the mask of another, either by mistake or
> else.
> 
> My suggestion is to keep things simple, the "SET" only receives the
> 'mask', and it only changes the mask for that particular fd (which you
> already did the hard work of allowing that). Seems to be less error prone.
> 
> At least in my mental model, I don't think much else is needed (we
> expose only a "SET" operation), at least from the UAPI side of things.
> 
> For "debugging", i.e. discovering which applications have what masks,
> then perhaps we could do it "on the side", for example, a debugfs entry
> that lists all open file descriptors and their masks. Just an idea.
> 
> What do you think?

Thank you very much for your input Vinicius. I really appreciate it.

I totally agree with your observations. I had already thought about that angle
myself, but I decided to go this route anyway because it was the only way I
could think of meeting all of Richard's requirements at that time.

Even if being error prone, being able to externally manipulate the channel
masks is the only way I can think of to make this feature backwards compatible
with existing software. One example of a piece of software that would need to
be updated to support multiple channels is linuxptp. If you try to start ts2phc
with multiple channels enabled and no masks, it refuses to work stating that
unwanted channels are present. This would be easy to fix, incorporating the
SET operation you mention, but it is still something that needs to be changed.

Now that I think of it, it is true that nothing prevents us from having both
methods available: the simple and safe, and the complicated and unsafe.

Even with that option, I also think that going exclusively with the safe
and simple route is better.

So, I wonder: Can we just do it and require changes in software that relies
on this driver, or should we maintain compatibility at all cost?

Thank you very much for sharing your knowledge and experience.