summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorKirill Smelkov <kirr@nexedi.com>2019-03-26 18:20:43 -0400
committerLinus Torvalds <torvalds@linux-foundation.org>2019-04-06 13:01:55 -0400
commit10dce8af34226d90fa56746a934f8da5dcdba3df (patch)
treede5db64904d350146b21e2af1014e2ad910dddc3
parentbe76865df56f22f29ab20e671143761d78ed09c8 (diff)
fs: stream_open - opener for stream-like files so that read and write can run simultaneously without deadlock
Commit 9c225f2655e3 ("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. commit 581d21a2d02a ("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 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. However even though 2006'th version of Linus's patch 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)", the 2014 version - the one that actually made it into the tree as 9c225f2655e3 - is doing so irregardless of whether a file is seekable or not. See https://lore.kernel.org/lkml/53022DB1.4070805@gmail.com/ https://lwn.net/Articles/180387 https://lwn.net/Articles/180396 for historic context. 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 ... 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 a7c1b990f715 ("fuse: implement nonseekable open") to support OSSPD. 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: See https://github.com/libfuse/osspd https://lwn.net/Articles/308445 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 their 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. 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> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
-rw-r--r--drivers/xen/xenbus/xenbus_dev_frontend.c4
-rw-r--r--fs/open.c18
-rw-r--r--fs/read_write.c5
-rw-r--r--include/linux/fs.h4
-rw-r--r--scripts/coccinelle/api/stream_open.cocci363
5 files changed, 389 insertions, 5 deletions
diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c
index c3e201025ef0..0782ff3c2273 100644
--- a/drivers/xen/xenbus/xenbus_dev_frontend.c
+++ b/drivers/xen/xenbus/xenbus_dev_frontend.c
@@ -622,9 +622,7 @@ static int xenbus_file_open(struct inode *inode, struct file *filp)
622 if (xen_store_evtchn == 0) 622 if (xen_store_evtchn == 0)
623 return -ENOENT; 623 return -ENOENT;
624 624
625 nonseekable_open(inode, filp); 625 stream_open(inode, filp);
626
627 filp->f_mode &= ~FMODE_ATOMIC_POS; /* cdev-style semantics */
628 626
629 u = kzalloc(sizeof(*u), GFP_KERNEL); 627 u = kzalloc(sizeof(*u), GFP_KERNEL);
630 if (u == NULL) 628 if (u == NULL)
diff --git a/fs/open.c b/fs/open.c
index f1c2f855fd43..a00350018a47 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1215,3 +1215,21 @@ int nonseekable_open(struct inode *inode, struct file *filp)
1215} 1215}
1216 1216
1217EXPORT_SYMBOL(nonseekable_open); 1217EXPORT_SYMBOL(nonseekable_open);
1218
1219/*
1220 * stream_open is used by subsystems that want stream-like file descriptors.
1221 * Such file descriptors are not seekable and don't have notion of position
1222 * (file.f_pos is always 0). Contrary to file descriptors of other regular
1223 * files, .read() and .write() can run simultaneously.
1224 *
1225 * stream_open never fails and is marked to return int so that it could be
1226 * directly used as file_operations.open .
1227 */
1228int stream_open(struct inode *inode, struct file *filp)
1229{
1230 filp->f_mode &= ~(FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE | FMODE_ATOMIC_POS);
1231 filp->f_mode |= FMODE_STREAM;
1232 return 0;
1233}
1234
1235EXPORT_SYMBOL(stream_open);
diff --git a/fs/read_write.c b/fs/read_write.c
index 177ccc3d405a..61b43ad7608e 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -560,12 +560,13 @@ ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_
560 560
561static inline loff_t file_pos_read(struct file *file) 561static inline loff_t file_pos_read(struct file *file)
562{ 562{
563 return file->f_pos; 563 return file->f_mode & FMODE_STREAM ? 0 : file->f_pos;
564} 564}
565 565
566static inline void file_pos_write(struct file *file, loff_t pos) 566static inline void file_pos_write(struct file *file, loff_t pos)
567{ 567{
568 file->f_pos = pos; 568 if ((file->f_mode & FMODE_STREAM) == 0)
569 file->f_pos = pos;
569} 570}
570 571
571ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count) 572ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8b42df09b04c..dd28e7679089 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -158,6 +158,9 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
158#define FMODE_OPENED ((__force fmode_t)0x80000) 158#define FMODE_OPENED ((__force fmode_t)0x80000)
159#define FMODE_CREATED ((__force fmode_t)0x100000) 159#define FMODE_CREATED ((__force fmode_t)0x100000)
160 160
161/* File is stream-like */
162#define FMODE_STREAM ((__force fmode_t)0x200000)
163
161/* File was opened by fanotify and shouldn't generate fanotify events */ 164/* File was opened by fanotify and shouldn't generate fanotify events */
162#define FMODE_NONOTIFY ((__force fmode_t)0x4000000) 165#define FMODE_NONOTIFY ((__force fmode_t)0x4000000)
163 166
@@ -3074,6 +3077,7 @@ extern loff_t no_seek_end_llseek_size(struct file *, loff_t, int, loff_t);
3074extern loff_t no_seek_end_llseek(struct file *, loff_t, int); 3077extern loff_t no_seek_end_llseek(struct file *, loff_t, int);
3075extern int generic_file_open(struct inode * inode, struct file * filp); 3078extern int generic_file_open(struct inode * inode, struct file * filp);
3076extern int nonseekable_open(struct inode * inode, struct file * filp); 3079extern int nonseekable_open(struct inode * inode, struct file * filp);
3080extern int stream_open(struct inode * inode, struct file * filp);
3077 3081
3078#ifdef CONFIG_BLOCK 3082#ifdef CONFIG_BLOCK
3079typedef void (dio_submit_t)(struct bio *bio, struct inode *inode, 3083typedef void (dio_submit_t)(struct bio *bio, struct inode *inode,
diff --git a/scripts/coccinelle/api/stream_open.cocci b/scripts/coccinelle/api/stream_open.cocci
new file mode 100644
index 000000000000..350145da7669
--- /dev/null
+++ b/scripts/coccinelle/api/stream_open.cocci
@@ -0,0 +1,363 @@
1// SPDX-License-Identifier: GPL-2.0
2// Author: Kirill Smelkov (kirr@nexedi.com)
3//
4// Search for stream-like files that are using nonseekable_open and convert
5// them to stream_open. A stream-like file is a file that does not use ppos in
6// its read and write. Rationale for the conversion is to avoid deadlock in
7// between read and write.
8
9virtual report
10virtual patch
11virtual explain // explain decisions in the patch (SPFLAGS="-D explain")
12
13// stream-like reader & writer - ones that do not depend on f_pos.
14@ stream_reader @
15identifier readstream, ppos;
16identifier f, buf, len;
17type loff_t;
18@@
19 ssize_t readstream(struct file *f, char *buf, size_t len, loff_t *ppos)
20 {
21 ... when != ppos
22 }
23
24@ stream_writer @
25identifier writestream, ppos;
26identifier f, buf, len;
27type loff_t;
28@@
29 ssize_t writestream(struct file *f, const char *buf, size_t len, loff_t *ppos)
30 {
31 ... when != ppos
32 }
33
34
35// a function that blocks
36@ blocks @
37identifier block_f;
38identifier wait_event =~ "^wait_event_.*";
39@@
40 block_f(...) {
41 ... when exists
42 wait_event(...)
43 ... when exists
44 }
45
46// stream_reader that can block inside.
47//
48// XXX wait_* can be called not directly from current function (e.g. func -> f -> g -> wait())
49// XXX currently reader_blocks supports only direct and 1-level indirect cases.
50@ reader_blocks_direct @
51identifier stream_reader.readstream;
52identifier wait_event =~ "^wait_event_.*";
53@@
54 readstream(...)
55 {
56 ... when exists
57 wait_event(...)
58 ... when exists
59 }
60
61@ reader_blocks_1 @
62identifier stream_reader.readstream;
63identifier blocks.block_f;
64@@
65 readstream(...)
66 {
67 ... when exists
68 block_f(...)
69 ... when exists
70 }
71
72@ reader_blocks depends on reader_blocks_direct || reader_blocks_1 @
73identifier stream_reader.readstream;
74@@
75 readstream(...) {
76 ...
77 }
78
79
80// file_operations + whether they have _any_ .read, .write, .llseek ... at all.
81//
82// XXX add support for file_operations xxx[N] = ... (sound/core/pcm_native.c)
83@ fops0 @
84identifier fops;
85@@
86 struct file_operations fops = {
87 ...
88 };
89
90@ has_read @
91identifier fops0.fops;
92identifier read_f;
93@@
94 struct file_operations fops = {
95 .read = read_f,
96 };
97
98@ has_read_iter @
99identifier fops0.fops;
100identifier read_iter_f;
101@@
102 struct file_operations fops = {
103 .read_iter = read_iter_f,
104 };
105
106@ has_write @
107identifier fops0.fops;
108identifier write_f;
109@@
110 struct file_operations fops = {
111 .write = write_f,
112 };
113
114@ has_write_iter @
115identifier fops0.fops;
116identifier write_iter_f;
117@@
118 struct file_operations fops = {
119 .write_iter = write_iter_f,
120 };
121
122@ has_llseek @
123identifier fops0.fops;
124identifier llseek_f;
125@@
126 struct file_operations fops = {
127 .llseek = llseek_f,
128 };
129
130@ has_no_llseek @
131identifier fops0.fops;
132@@
133 struct file_operations fops = {
134 .llseek = no_llseek,
135 };
136
137@ has_mmap @
138identifier fops0.fops;
139identifier mmap_f;
140@@
141 struct file_operations fops = {
142 .mmap = mmap_f,
143 };
144
145@ has_copy_file_range @
146identifier fops0.fops;
147identifier copy_file_range_f;
148@@
149 struct file_operations fops = {
150 .copy_file_range = copy_file_range_f,
151 };
152
153@ has_remap_file_range @
154identifier fops0.fops;
155identifier remap_file_range_f;
156@@
157 struct file_operations fops = {
158 .remap_file_range = remap_file_range_f,
159 };
160
161@ has_splice_read @
162identifier fops0.fops;
163identifier splice_read_f;
164@@
165 struct file_operations fops = {
166 .splice_read = splice_read_f,
167 };
168
169@ has_splice_write @
170identifier fops0.fops;
171identifier splice_write_f;
172@@
173 struct file_operations fops = {
174 .splice_write = splice_write_f,
175 };
176
177
178// file_operations that is candidate for stream_open conversion - it does not
179// use mmap and other methods that assume @offset access to file.
180//
181// XXX for simplicity require no .{read/write}_iter and no .splice_{read/write} for now.
182// XXX maybe_steam.fops cannot be used in other rules - it gives "bad rule maybe_stream or bad variable fops".
183@ maybe_stream depends on (!has_llseek || has_no_llseek) && !has_mmap && !has_copy_file_range && !has_remap_file_range && !has_read_iter && !has_write_iter && !has_splice_read && !has_splice_write @
184identifier fops0.fops;
185@@
186 struct file_operations fops = {
187 };
188
189
190// ---- conversions ----
191
192// XXX .open = nonseekable_open -> .open = stream_open
193// XXX .open = func -> openfunc -> nonseekable_open
194
195// read & write
196//
197// if both are used in the same file_operations together with an opener -
198// under that conditions we can use stream_open instead of nonseekable_open.
199@ fops_rw depends on maybe_stream @
200identifier fops0.fops, openfunc;
201identifier stream_reader.readstream;
202identifier stream_writer.writestream;
203@@
204 struct file_operations fops = {
205 .open = openfunc,
206 .read = readstream,
207 .write = writestream,
208 };
209
210@ report_rw depends on report @
211identifier fops_rw.openfunc;
212position p1;
213@@
214 openfunc(...) {
215 <...
216 nonseekable_open@p1
217 ...>
218 }
219
220@ script:python depends on report && reader_blocks @
221fops << fops0.fops;
222p << report_rw.p1;
223@@
224coccilib.report.print_report(p[0],
225 "ERROR: %s: .read() can deadlock .write(); change nonseekable_open -> stream_open to fix." % (fops,))
226
227@ script:python depends on report && !reader_blocks @
228fops << fops0.fops;
229p << report_rw.p1;
230@@
231coccilib.report.print_report(p[0],
232 "WARNING: %s: .read() and .write() have stream semantic; safe to change nonseekable_open -> stream_open." % (fops,))
233
234
235@ explain_rw_deadlocked depends on explain && reader_blocks @
236identifier fops_rw.openfunc;
237@@
238 openfunc(...) {
239 <...
240- nonseekable_open
241+ nonseekable_open /* read & write (was deadlock) */
242 ...>
243 }
244
245
246@ explain_rw_nodeadlock depends on explain && !reader_blocks @
247identifier fops_rw.openfunc;
248@@
249 openfunc(...) {
250 <...
251- nonseekable_open
252+ nonseekable_open /* read & write (no direct deadlock) */
253 ...>
254 }
255
256@ patch_rw depends on patch @
257identifier fops_rw.openfunc;
258@@
259 openfunc(...) {
260 <...
261- nonseekable_open
262+ stream_open
263 ...>
264 }
265
266
267// read, but not write
268@ fops_r depends on maybe_stream && !has_write @
269identifier fops0.fops, openfunc;
270identifier stream_reader.readstream;
271@@
272 struct file_operations fops = {
273 .open = openfunc,
274 .read = readstream,
275 };
276
277@ report_r depends on report @
278identifier fops_r.openfunc;
279position p1;
280@@
281 openfunc(...) {
282 <...
283 nonseekable_open@p1
284 ...>
285 }
286
287@ script:python depends on report @
288fops << fops0.fops;
289p << report_r.p1;
290@@
291coccilib.report.print_report(p[0],
292 "WARNING: %s: .read() has stream semantic; safe to change nonseekable_open -> stream_open." % (fops,))
293
294@ explain_r depends on explain @
295identifier fops_r.openfunc;
296@@
297 openfunc(...) {
298 <...
299- nonseekable_open
300+ nonseekable_open /* read only */
301 ...>
302 }
303
304@ patch_r depends on patch @
305identifier fops_r.openfunc;
306@@
307 openfunc(...) {
308 <...
309- nonseekable_open
310+ stream_open
311 ...>
312 }
313
314
315// write, but not read
316@ fops_w depends on maybe_stream && !has_read @
317identifier fops0.fops, openfunc;
318identifier stream_writer.writestream;
319@@
320 struct file_operations fops = {
321 .open = openfunc,
322 .write = writestream,
323 };
324
325@ report_w depends on report @
326identifier fops_w.openfunc;
327position p1;
328@@
329 openfunc(...) {
330 <...
331 nonseekable_open@p1
332 ...>
333 }
334
335@ script:python depends on report @
336fops << fops0.fops;
337p << report_w.p1;
338@@
339coccilib.report.print_report(p[0],
340 "WARNING: %s: .write() has stream semantic; safe to change nonseekable_open -> stream_open." % (fops,))
341
342@ explain_w depends on explain @
343identifier fops_w.openfunc;
344@@
345 openfunc(...) {
346 <...
347- nonseekable_open
348+ nonseekable_open /* write only */
349 ...>
350 }
351
352@ patch_w depends on patch @
353identifier fops_w.openfunc;
354@@
355 openfunc(...) {
356 <...
357- nonseekable_open
358+ stream_open
359 ...>
360 }
361
362
363// no read, no write - don't change anything