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: <20231006233537.7721-1-reibax@gmail.com>
Date: Sat,  7 Oct 2023 01:35:37 +0200
From: Xabier Marquiegui <reibax@...il.com>
To: reibax@...il.com
Cc: chrony-dev@...ony.tuxfamily.org,
	davem@...emloft.net,
	horms@...nel.org,
	jstultz@...gle.com,
	mlichvar@...hat.com,
	netdev@...r.kernel.org,
	ntp-lists@...tcorallo.com,
	richardcochran@...il.com,
	rrameshbabu@...dia.com,
	shuah@...nel.org,
	tglx@...utronix.de,
	vinicius.gomes@...el.com
Subject: Re: [PATCH net-next v4 4/6] ptp: support event queue reader channel masks

Simon Horman said:
> Hi Xabier,
>
> queue appears to be leaked here.
>
> As flagged by Smatch.

Nice catch Simon. Thank you very much. I think I know how to fix it. I
will keep it in mind for the next revision.

Vinicius Costa Gomes said:
> Sorry that I only noticed a (possible) change in behavior now.
>
> Before this series, when there was a single queue, events where
> accumulated until the application reads the fd associated with the PTP
> device. i.e. it doesn't matter when the application calls open().

You are totally correct about that observation. I had never thought of
this angle until you mentioned it. Thank you for bringing it up.

> AFter this series events, are only accumulated after the queue
> associated with that fd is created, i.e. after open(). Events that
> happened before open() are lost (is this true? are we leaking them?).

Old events are indeed lost for a new reader, but I don't see how that
could be causing a leak. The way it works is, we always have at least
one queue: the one corresponding to sysfs.

Whenever a new reader accesses the device, a new queue is created and
starts to get fed with new coming timestamps alongside the rest of
existing queues.

> Is this a desired/wanted change? Is it possible that we have
> applications that depend on the "old" behavior?

I would really like to hear the voice of more experience people on this.
On my limited experience this is a non-issue because I can control the
sequencing and I am sure to have the reader ready before I trigger events,
but you might be right that there might be some use-cases I didn't imagine
that could be affected by this change in behavior.

We could tweak the system a little bit by having an additional reference
fifo with no readers. Whenever a new ptp_open happens, I could just copy
the entire reference fifo to the new one. I guess this would bring back
the need to have the fifo mutex.

If this idea works we could be maintaining the same functionality, at the
cost of making the system be more complex and slower. Is it worth it?

I look forward to hearing opinions on this. Thank you everyone for your
feedback.


Powered by blists - more mailing lists