linux-kernel - Re: [PATCH] pps: add epoll support

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] [day] [month] [year] [list]
Message-ID: <97190cf3-259e-4a3c-93c1-83a86416ff3a@enneenne.com>
Date: Mon, 24 Feb 2025 18:38:49 +0100
From: Rodolfo Giometti <giometti@...eenne.com>
To: Denis OSTERLAND-HEIM <denis.osterland@...hl.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pps: add epoll support

On 24/02/25 12:38, Denis OSTERLAND-HEIM wrote:
> Hi,
> 
> I tested it today and that patch creates not the expected behavior.

[snip]

> The idea is to use poll to wait for the next data become available.
> The should poll work like `wait_event_interruptible(pps->queue, ev != pps->last_ev)`,
> where `poll_wait(file, &pps->queue, wait)` already does the first part,
> but the condition `ev != pps->last_ev` is missing.
> 
> Poll shall return 0 if ev == ps->last_ev
> and shall return EPOLLIN if ev != ps->last_ev; EPOLLRDNORM is equivalent[^1] so better return both
> 
> In case of ioctl(PPS_FETCH) ev is stored on the stack.
> If two applications access one pps device, they both get the right result, because the wait_event uses the ev from their stack.
> To do so with poll() ev needs to be available via file, because every applications opens a sepate file and the file is passed to poll.
> That is what my patch does.
> It adds a per file storage so that poll has the ev value of the last fetch to compare.

I agree, but are you sure that your solution will save you in case of fork()? 
So, why don't simply add a new variable as below?

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 6a02245ea35f..dded1452c3a8 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -40,8 +40,11 @@ static __poll_t pps_cdev_poll(struct file *file, poll_table 
*wait)
         struct pps_device *pps = file->private_data;

         poll_wait(file, &pps->queue, wait);
+       if (pps->info.mode & PPS_CANWAIT)
+               if (pps->fetched_ev != pps->last_ev)
+                       return EPOLLIN | EPOLLRDNORM;

-       return EPOLLIN | EPOLLRDNORM;
+       return 0;
  }

  static int pps_cdev_fasync(int fd, struct file *file, int on)
@@ -186,9 +195,11 @@ static long pps_cdev_ioctl(struct file *file,
                 if (err)
                         return err;

-               /* Return the fetched timestamp */
+               /* Return the fetched timestamp and save last fetched event  */
                 spin_lock_irq(&pps->lock);

+               pps->last_fetched_ev = pps->last_ev;
+
                 fdata.info.assert_sequence = pps->assert_sequence;
                 fdata.info.clear_sequence = pps->clear_sequence;
                 fdata.info.assert_tu = pps->assert_tu;
@@ -272,9 +283,11 @@ static long pps_cdev_compat_ioctl(struct file *file,
                 if (err)
                         return err;

-               /* Return the fetched timestamp */
+               /* Return the fetched timestamp and save last fetched event  */
                 spin_lock_irq(&pps->lock);

+               pps->last_fetched_ev = pps->last_ev;
+
                 compat.info.assert_sequence = pps->assert_sequence;
                 compat.info.clear_sequence = pps->clear_sequence;
                 compat.info.current_mode = pps->current_mode;
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index c7abce28ed29..aab0aebb529e 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -52,6 +52,7 @@ struct pps_device {
         int current_mode;                       /* PPS mode at event time */

         unsigned int last_ev;                   /* last PPS event id */
+       unsigned int last_fetched_ev;           /* last fetched PPS event id */
         wait_queue_head_t queue;                /* PPS event queue */

         unsigned int id;                        /* PPS source unique ID */

> If you want to avoid this extra alloc and derefers, we may use file position (file.f_pos) to store last fetched ev value.
> The pps does not provide read/write, so f_pos is unused anyway.
> 
> I am a little bit puzzeled about your second diff.
> Every pps client that uses pps_event() supports WAITEVENT, because this is in the generic part.
> To me not using pps_event() but reimplement parts of pps_event() seems to be a bad idea.
> That’s why I thing that this diff is not needed.

All clients specify their PPS information within the struct pps_source_info, and 
if for some reason one source doesn't add the PPS_CANWAIT flag, we should 
properly support this setting.

However, this is related to the poll() support, and we can defer it for the moment.

Thank you so much for your suggestions and tests! :)

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@...eenne.com
Linux Device Driver                          giometti@...ux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming