netdev - RE: Issues in error queue polling

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: <02874ECE860811409154E81DA85FBB5884CDD3CC@ORSMSX115.amr.corp.intel.com>
Date:   Thu, 18 Oct 2018 21:54:15 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     "pasqualirb@...il.com" <pasqualirb@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "Gomes, Vinicius" <vinicius.gomes@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: RE: Issues in error queue polling

Hi,

> -----Original Message-----
> From: Ricardo Biehl Pasquali [mailto:pasqualirb@...il.com]
> Sent: Thursday, October 18, 2018 11:27 AM
> To: netdev@...r.kernel.org
> Cc: Keller, Jacob E <jacob.e.keller@...el.com>; Gomes, Vinicius
> <vinicius.gomes@...el.com>; davem@...emloft.net
> Subject: Issues in error queue polling
> 
> The commit 7d4c04fc170087119727 ("net: add option to enable
> error queue packets waking select") (2013-03-28) introduced
> SO_SELECT_ERR_QUEUE, which masks POLLPRI with POLLERR event
> return in some socket poll callbacks.
> 
> POLLERR event issued with sock_queue_err_skb() did not wake
> up a poll when POLLERR is the only requested event because
> sk_data_ready() (sock_def_readable()) was used and it
> doesn't mask POLLERR in poll wake up:
> 

Right.

> wake_up_interruptible_sync_poll(&wq->wait,
>                                 EPOLLIN | EPOLLPRI |
>                                 EPOLLRDNORM | EPOLLRDBAND);
> 
> If POLLIN or POLLPRI are requested, for example, poll does
> wake up.
> 
> POLLERR wakeup by requesting POLLPRI is possible without
> set SO_SELECT_ERR_QUEUE. All the option does is masking
> POLLPRI as a returned event before poll returns. poll
> would return anyway because of POLLERR.
> 

Yes. The problem being that the application thread not being ready to handle POLLPRI, so they want to avoid the application waking up to an event.

> Also, the sentence "[...] enable software to wait on error
> queue packets without waking up for regular data on the
> socket." from the above commit is not true.
> 

Not entirely true but...

> A POLLIN event issued via sock_def_readable() wakes up
> threads waiting for POLLPRI, and vice versa. However,
> poll() does not return, sleeping again, as the requested
> events do not match events.
> 

The thread wakes up, but the application handling the events doesn't because the thread goes right back to sleep.

> The commit 6e5d58fdc9bedd0255a8 ("skbuff: Fix not waking
> applications when errors are enqueued") (2018-03-14) make
> POLLERR alone wake up poll. It replaces sk_data_ready()
> (sock_def_readable()) with sk_error_report()
> (sock_def_error_report()). This makes "POLLERR wake up by
> requesting POLLPRI" obsolete.
> 

Yep, this is a better solution, and I wish it had been thought of before we introduced SO_SELECT_ERR_QUEUE.

> Rationale:
> 
> POLLIN-only and POLLERR-only wake up are useful when there
> is a receiving thread, a sending thread, and a thread that
> get transmit timestamps. The thread polling on POLLERR will
> not wake up when regular data arrives (POLLIN). The thread
> polling on POLLIN will not wake up when tx timestamps are
> ready (POLLERR).

Right. This is the goal for applications like ptp4l.

> 
> One solution is adding an option that disable POLLERR as
> requested event. This is in the Virtual File System
> subsystem, not in the network, though.
> 
> This solves the problem of waking up other threads that
> not interested in error queue. Thus allowing a separate
> thread take care of error queue (useful for receiving
> transmit timestamps).

Yes, this makes sense to me.

Thanks,
Jake