lkml.org 
[lkml]   [2019]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 1/3] fs: stream_open - opener for stream-like files so that read and write can run simultaneously without deadlock
    From
    Date
    On 26/03/2019 23:20, Kirill Smelkov wrote:
    > Commit 9c225f2655 (vfs: atomic f_pos accesses as per POSIX) added locking for
    > file.f_pos access and in particular made concurrent read and write not possible
    > - now both those functions take f_pos lock for the whole run, and so if e.g. a
    > read is blocked waiting for data, write will deadlock waiting for that read to
    > complete. This caused regression for stream-like files where previously read
    > and write could run simultaneously, but after that patch could not do so
    > anymore. See e.g. 581d21a2d0 (xenbus: fix deadlock on writes to /proc/xen/xenbus)
    > which fixes such regression for particular case of /proc/xen/xenbus.
    >
    > The patch that added f_pos lock in 2014 (see https://lkml.org/lkml/2014/2/17/324
    > for background discussion) did so to guarantee POSIX thread safety for
    > read/write/lseek and added the locking to file descriptors of all regular
    > files. In 2014 that thread-safety problem was not new as it was already discussed
    > earlier in 2006: https://lwn.net/Articles/180387. However even though 2006'th
    > version of Linus's patch (https://lwn.net/Articles/180396) was adding f_pos
    > locking "only for files that are marked seekable with FMODE_LSEEK (thus avoiding
    > the stream-like objects like pipes and sockets)", 2014'th version - the one that
    > actually made it into the tree as 9c225f2655 - is doing so irregardless of whether
    > a file is seekable or not. The reason that it did so is, probably, that there are
    > many files that are marked non-seekable, but e.g. their read implementation
    > actually depends on knowing current position to correctly handle the read. Some
    > examples:
    >
    > kernel/power/user.c snapshot_read
    > fs/debugfs/file.c u32_array_read
    > fs/fuse/control.c fuse_conn_waiting_read + ...
    > drivers/hwmon/asus_atk0110.c atk_debugfs_ggrp_read
    > arch/s390/hypfs/inode.c hypfs_read_iter
    > ...
    >
    > In despite that, many nonseekable_open users implement read and write with pure
    > stream semantics - they don't depend on passed ppos at all. And for those cases
    > where read could wait for something inside, it creates a situation similar to
    > xenbus - the write could be never made to go until read is done, and read is
    > waiting for some, potentially external, event, for potentially unbounded time
    > -> deadlock. Besides xenbus, there are 14 such places in the kernel that I've
    > found with semantic patch (see below):
    >
    > drivers/xen/evtchn.c:667:8-24: ERROR: evtchn_fops: .read() can deadlock .write()
    > drivers/isdn/capi/capi.c:963:8-24: ERROR: capi_fops: .read() can deadlock .write()
    > drivers/input/evdev.c:527:1-17: ERROR: evdev_fops: .read() can deadlock .write()
    > drivers/char/pcmcia/cm4000_cs.c:1685:7-23: ERROR: cm4000_fops: .read() can deadlock .write()
    > net/rfkill/core.c:1146:8-24: ERROR: rfkill_fops: .read() can deadlock .write()
    > drivers/s390/char/fs3270.c:488:1-17: ERROR: fs3270_fops: .read() can deadlock .write()
    > drivers/usb/misc/ldusb.c:310:1-17: ERROR: ld_usb_fops: .read() can deadlock .write()
    > drivers/hid/uhid.c:635:1-17: ERROR: uhid_fops: .read() can deadlock .write()
    > net/batman-adv/icmp_socket.c:80:1-17: ERROR: batadv_fops: .read() can deadlock .write()
    > drivers/media/rc/lirc_dev.c:198:1-17: ERROR: lirc_fops: .read() can deadlock .write()
    > drivers/leds/uleds.c:77:1-17: ERROR: uleds_fops: .read() can deadlock .write()
    > drivers/input/misc/uinput.c:400:1-17: ERROR: uinput_fops: .read() can deadlock .write()
    > drivers/infiniband/core/user_mad.c:985:7-23: ERROR: umad_fops: .read() can deadlock .write()
    > drivers/gnss/core.c:45:1-17: ERROR: gnss_fops: .read() can deadlock .write()
    >
    > In addition to the cases above another regression caused by f_pos locking is
    > that now FUSE filesystems that implement open with FOPEN_NONSEEKABLE flag, can
    > no longer implement bidirectional stream-like files - for the same reason
    > as above e.g. read can deadlock write locking on file.f_pos in the kernel.
    > FUSE's FOPEN_NONSEEKABLE was added in 2008 in a7c1b990f7 (fuse: implement
    > nonseekable open) to support OSSPD (https://github.com/libfuse/osspd;
    > https://lwn.net/Articles/308445). OSSPD implements /dev/dsp in userspace with
    > FOPEN_NONSEEKABLE flag, with corresponding read and write routines not
    > depending on current position at all, and with both read and write being
    > potentially blocking operations:
    >
    > https://github.com/libfuse/osspd/blob/14a9cff0/osspd.c#L1406
    > https://github.com/libfuse/osspd/blob/14a9cff0/osspd.c#L1438-L1477
    > https://github.com/libfuse/osspd/blob/14a9cff0/osspd.c#L1479-L1510
    >
    > Corresponding libfuse example/test also describes FOPEN_NONSEEKABLE as
    > "somewhat pipe-like files ..." with read handler not using offset. However
    > that test implements only read without write and cannot exercise the deadlock
    > scenario:
    >
    > https://github.com/libfuse/libfuse/blob/fuse-3.4.2-3-ga1bff7d/example/poll.c#L124-L131
    > https://github.com/libfuse/libfuse/blob/fuse-3.4.2-3-ga1bff7d/example/poll.c#L146-L163
    > https://github.com/libfuse/libfuse/blob/fuse-3.4.2-3-ga1bff7d/example/poll.c#L209-L216
    >
    > I've actually hit the read vs write deadlock for real while implementing my
    > FUSE filesystem where there is /head/watch file, for which open creates
    > separate bidirectional socket-like stream in between filesystem and its user
    > with both read and write being later performed simultaneously. And there it is
    > semantically not easy to split the stream into two separate read-only and
    > write-only channels:
    >
    > https://lab.nexedi.com/kirr/wendelin.core/blob/f13aa600/wcfs/wcfs.go#L88-169
    >
    > Let's fix this regression. The plan is:
    >
    > 1. We can't change nonseekable_open to include &~FMODE_ATOMIC_POS - doing so would
    > break many in-kernel nonseekable_open users which actually use ppos in
    > read/write handlers.
    >
    > 2. Add stream_open() to kernel to open stream-like non-seekable file descriptors.
    > Read and write on such file descriptors would never use nor change ppos. And
    > with that property on stream-like files read and write will be running without
    > taking f_pos lock - i.e. read and write could be running simultaneously.
    >
    > 3. With semantic patch search and convert to stream_open all in-kernel
    > nonseekable_open users for which read and write actually do not depend on ppos and
    > where there is no other methods in file_operations which assume @offset access.
    >
    > 4. Add FOPEN_STREAM to fs/fuse/ and open in-kernel file-descriptors via steam_open
    > if that bit is present in filesystem open reply.
    >
    > It was tempting to change fs/fuse/ open handler to use stream_open instead of
    > nonseekable_open on just FOPEN_NONSEEKABLE flags, but grepping through Debian
    > codesearch shows users of FOPEN_NONSEEKABLE, and in particular GVFS which actually
    > uses offset in its read and write handlers
    >
    > https://codesearch.debian.net/search?q=-%3Enonseekable+%3D
    > https://gitlab.gnome.org/GNOME/gvfs/blob/1.40.0-6-gcbc54396/client/gvfsfusedaemon.c#L1080
    > https://gitlab.gnome.org/GNOME/gvfs/blob/1.40.0-6-gcbc54396/client/gvfsfusedaemon.c#L1247-1346
    > https://gitlab.gnome.org/GNOME/gvfs/blob/1.40.0-6-gcbc54396/client/gvfsfusedaemon.c#L1399-1481
    >
    > so if we would do such a change it will break a real user.
    >
    > 5. Add stream_open and FOPEN_STREAM handling to stable kernels starting from
    > v3.14+ (the kernel where 9c225f2655 first appeared). This will allow to patch
    > OSSPD and other FUSE filesystems that provide stream-like files to return
    > FOPEN_STREAM | FOPEN_NONSEEKABLE in open handler and this way avoid the deadlock on
    > all kernel versions. This should work because fs/fuse/ ignores unknown open
    > flags returned from a filesystem and so passing FOPEN_STREAM to a kernel that
    > is not aware of this flag cannot hurt. In turn the kernel that is not aware of
    > FOPEN_STREAM will be < v3.14 where just FOPEN_NONSEEKABLE is sufficient to
    > implement streams without read vs write deadlock.
    >
    > This patch: adds stream_open, converts /proc/xen/xenbus to it and adds semantic
    > patch to automatically locate in-kernel places that are either required to be
    > converted due to read vs write deadlock, or that are just safe to be converted
    > because read and write do not use ppos and there are no other funky methods in
    > file_operations.
    >
    > Followup patches are:
    >
    > - apply the result of semantic patch;
    > - add FOPEN_STREAM to fs/fuse.
    >
    > Regarding semantic patch I've verified each generated change manually - that it is
    > correct to convert - and each other nonseekable_open instance left - that it is
    > either not correct to convert there, or that it is not converted due to current
    > stream_open.cocci limitations. The script also does not convert files that should
    > be valid to convert, but that currently have .llseek = noop_llseek or
    > generic_file_llseek for unknown reason despite file being opened with
    > nonseekable_open (e.g. drivers/input/mousedev.c)
    >
    > Cc: Michael Kerrisk <mtk.manpages@gmail.com>
    > Cc: Yongzhi Pan <panyongzhi@gmail.com>
    > Cc: Jonathan Corbet <corbet@lwn.net>
    > Cc: David Vrabel <david.vrabel@citrix.com>
    > Cc: Juergen Gross <jgross@suse.com>
    > Cc: Miklos Szeredi <miklos@szeredi.hu>
    > Cc: Tejun Heo <tj@kernel.org>
    > Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
    > Cc: Arnd Bergmann <arnd@arndb.de>
    > Cc: Christoph Hellwig <hch@lst.de>
    > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    > Cc: Julia Lawall <Julia.Lawall@lip6.fr>
    > Cc: Nikolaus Rath <Nikolaus@rath.org>
    > Cc: Han-Wen Nienhuys <hanwen@google.com>
    > Signed-off-by: Kirill Smelkov <kirr@nexedi.com>

    For the Xen changes: Reviewed-by: Juergen Gross <jgross@suse.com>


    Juergen

    \
     
     \ /
      Last update: 2019-03-27 17:58    [W:2.617 / U:0.408 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site