netdev - Re: [PATCH net v2] ice: make writes to /dev/gnssX synchronous

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: <ZHB4Lk2NlY5C9nKW@corigine.com>
Date: Fri, 26 May 2023 11:13:18 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Michal Schmidt <mschmidt@...hat.com>
Cc: intel-wired-lan@...ts.osuosl.org,
	Karol Kolacinski <karol.kolacinski@...el.com>,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
	Michal Michalik <michal.michalik@...el.com>,
	Petr Oros <poros@...hat.com>, netdev@...r.kernel.org,
	Johan Hovold <johan@...nel.org>
Subject: Re: [PATCH net v2] ice: make writes to /dev/gnssX synchronous

On Thu, May 25, 2023 at 12:04:46PM +0200, Michal Schmidt wrote:
> The current ice driver's GNSS write implementation buffers writes and
> works through them asynchronously in a kthread. That's bad because:
>  - The GNSS write_raw operation is supposed to be synchronous[1][2].
>  - There is no upper bound on the number of pending writes.
>    Userspace can submit writes much faster than the driver can process,
>    consuming unlimited amounts of kernel memory.
> 
> A patch that's currently on review[3] ("[v3,net] ice: Write all GNSS
> buffers instead of first one") would add one more problem:
>  - The possibility of waiting for a very long time to flush the write
>    work when doing rmmod, softlockups.
> 
> To fix these issues, simplify the implementation: Drop the buffering,
> the write_work, and make the writes synchronous.
> 
> I tested this with gpsd and ubxtool.
> 
> [1] https://events19.linuxfoundation.org/wp-content/uploads/2017/12/The-GNSS-Subsystem-Johan-Hovold-Hovold-Consulting-AB.pdf
>     "User interface" slide.
> [2] A comment in drivers/gnss/core.c:gnss_write():
>         /* Ignoring O_NONBLOCK, write_raw() is synchronous. */
> [3] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20230217120541.16745-1-karol.kolacinski@intel.com/
> 
> Fixes: d6b98c8d242a ("ice: add write functionality for GNSS TTY")
> Signed-off-by: Michal Schmidt <mschmidt@...hat.com>
> ---
> v2: No real change. Just rebased, per Michal Michalik's request.

Reviewed-by: Simon Horman <simon.horman@...igine.com>

Added Johan Hovold to CC list in case he has any feedback.