[PATCH 0/3] stream_open bits for Linux 5.2

[PATCH 0/3] stream_open bits for Linux 5.2

[Kirill Smelkov]

Linus,

Please consider applying the following stream_open related patches:

- The first one removes unnecessary double nonseekable_open from
  drivers/char/dtlk.c as noticed by Pavel Machek while reviewing
  nonseekable_open -> stream_open mass conversion.

- The second one is the mass conversion patch promised in 10dce8af3422 ("fs:
  stream_open - opener for stream-like files so that read and write can run
  simultaneously without deadlock") and is automatically generated by running

	$ make coccicheck MODE=patch COCCI=scripts/coccinelle/api/stream_open.cocci

  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. More details on this in the patch.

- The third patch changes VFS to pass ppos=NULL into .read/.write for files
  that declare themselves streams. It was suggested by Rasmus Villemoes and makes
  sure that if ppos starts to be erroneously used in a stream file, such bug
  won't go unnoticed and will produce an oops instead of creating illusion of
  position change being taken into account.

  Note: this patch does not conflict with "fuse: Add FOPEN_STREAM to use
  stream_open()" that will be hopefully coming via FUSE tree, because fs/fuse/
  uses new-style .read_iter/.write_iter, and for these accessors position is
  still passed as non-pointer kiocb.ki_pos .

I was hoping for the patches to be picked up into VFS tree, but since that did
not happenned for some reason I'm sending them to you directly.

Maybe it will help: the patches can be also pulled from here:

	git pull https://lab.nexedi.com/kirr/linux.git y/stream_open-5.2


Thanks beforehand,
Kirill

Kirill Smelkov (3):
  dtlk: remove double call to nonseekable_open
  *: convert stream-like files from nonseekable_open -> stream_open
  vfs: pass ppos=NULL to .read()/.write() of FMODE_STREAM files

 arch/powerpc/platforms/52xx/mpc52xx_gpt.c |   2 +-
 arch/powerpc/platforms/cell/spufs/file.c  |   2 +-
 arch/um/drivers/harddog_kern.c            |   2 +-
 arch/x86/kernel/cpu/microcode/core.c      |   2 +-
 drivers/char/ds1620.c                     |   2 +-
 drivers/char/dtlk.c                       |   3 +-
 drivers/char/ipmi/ipmi_watchdog.c         |   2 +-
 drivers/char/pcmcia/cm4000_cs.c           |   2 +-
 drivers/char/pcmcia/scr24x_cs.c           |   2 +-
 drivers/char/tb0219.c                     |   2 +-
 drivers/firewire/nosy.c                   |   2 +-
 drivers/gnss/core.c                       |   2 +-
 drivers/hid/uhid.c                        |   2 +-
 drivers/hwmon/fschmd.c                    |   2 +-
 drivers/hwmon/w83793.c                    |   2 +-
 drivers/infiniband/core/ucm.c             |   2 +-
 drivers/infiniband/core/ucma.c            |   2 +-
 drivers/infiniband/core/user_mad.c        |   2 +-
 drivers/infiniband/core/uverbs_main.c     |   2 +-
 drivers/input/evdev.c                     |   2 +-
 drivers/input/joydev.c                    |   2 +-
 drivers/input/misc/uinput.c               |   2 +-
 drivers/isdn/capi/capi.c                  |   2 +-
 drivers/leds/uleds.c                      |   2 +-
 drivers/media/rc/lirc_dev.c               |   2 +-
 drivers/pci/switch/switchtec.c            |   2 +-
 drivers/platform/chrome/cros_ec_debugfs.c |   2 +-
 drivers/rtc/rtc-ds1374.c                  |   2 +-
 drivers/rtc/rtc-m41t80.c                  |   2 +-
 drivers/s390/char/fs3270.c                |   2 +-
 drivers/s390/char/tape_char.c             |   2 +-
 drivers/s390/char/zcore.c                 |   2 +-
 drivers/s390/crypto/zcrypt_api.c          |   2 +-
 drivers/spi/spidev.c                      |   2 +-
 drivers/staging/pi433/pi433_if.c          |   2 +-
 drivers/usb/misc/ldusb.c                  |   2 +-
 drivers/watchdog/acquirewdt.c             |   2 +-
 drivers/watchdog/advantechwdt.c           |   2 +-
 drivers/watchdog/alim1535_wdt.c           |   2 +-
 drivers/watchdog/alim7101_wdt.c           |   2 +-
 drivers/watchdog/ar7_wdt.c                |   2 +-
 drivers/watchdog/at91rm9200_wdt.c         |   2 +-
 drivers/watchdog/ath79_wdt.c              |   2 +-
 drivers/watchdog/bcm63xx_wdt.c            |   2 +-
 drivers/watchdog/cpu5wdt.c                |   2 +-
 drivers/watchdog/cpwd.c                   |   2 +-
 drivers/watchdog/eurotechwdt.c            |   2 +-
 drivers/watchdog/f71808e_wdt.c            |   2 +-
 drivers/watchdog/gef_wdt.c                |   2 +-
 drivers/watchdog/geodewdt.c               |   2 +-
 drivers/watchdog/ib700wdt.c               |   2 +-
 drivers/watchdog/ibmasr.c                 |   2 +-
 drivers/watchdog/indydog.c                |   2 +-
 drivers/watchdog/intel_scu_watchdog.c     |   2 +-
 drivers/watchdog/iop_wdt.c                |   2 +-
 drivers/watchdog/it8712f_wdt.c            |   2 +-
 drivers/watchdog/ixp4xx_wdt.c             |   2 +-
 drivers/watchdog/ks8695_wdt.c             |   2 +-
 drivers/watchdog/m54xx_wdt.c              |   2 +-
 drivers/watchdog/machzwd.c                |   2 +-
 drivers/watchdog/mixcomwd.c               |   2 +-
 drivers/watchdog/mtx-1_wdt.c              |   2 +-
 drivers/watchdog/mv64x60_wdt.c            |   2 +-
 drivers/watchdog/nuc900_wdt.c             |   2 +-
 drivers/watchdog/nv_tco.c                 |   2 +-
 drivers/watchdog/pc87413_wdt.c            |   2 +-
 drivers/watchdog/pcwd.c                   |   4 +-
 drivers/watchdog/pcwd_pci.c               |   4 +-
 drivers/watchdog/pcwd_usb.c               |   4 +-
 drivers/watchdog/pika_wdt.c               |   2 +-
 drivers/watchdog/pnx833x_wdt.c            |   2 +-
 drivers/watchdog/rc32434_wdt.c            |   2 +-
 drivers/watchdog/rdc321x_wdt.c            |   2 +-
 drivers/watchdog/riowd.c                  |   2 +-
 drivers/watchdog/sa1100_wdt.c             |   2 +-
 drivers/watchdog/sb_wdog.c                |   2 +-
 drivers/watchdog/sbc60xxwdt.c             |   2 +-
 drivers/watchdog/sbc7240_wdt.c            |   2 +-
 drivers/watchdog/sbc8360.c                |   2 +-
 drivers/watchdog/sbc_epx_c3.c             |   2 +-
 drivers/watchdog/sbc_fitpc2_wdt.c         |   2 +-
 drivers/watchdog/sc1200wdt.c              |   2 +-
 drivers/watchdog/sc520_wdt.c              |   2 +-
 drivers/watchdog/sch311x_wdt.c            |   2 +-
 drivers/watchdog/scx200_wdt.c             |   2 +-
 drivers/watchdog/smsc37b787_wdt.c         |   2 +-
 drivers/watchdog/w83877f_wdt.c            |   2 +-
 drivers/watchdog/w83977f_wdt.c            |   2 +-
 drivers/watchdog/wafer5823wdt.c           |   2 +-
 drivers/watchdog/watchdog_dev.c           |   2 +-
 drivers/watchdog/wdrtas.c                 |   4 +-
 drivers/watchdog/wdt.c                    |   4 +-
 drivers/watchdog/wdt285.c                 |   2 +-
 drivers/watchdog/wdt977.c                 |   2 +-
 drivers/watchdog/wdt_pci.c                |   4 +-
 drivers/xen/evtchn.c                      |   2 +-
 fs/open.c                                 |   5 +-
 fs/read_write.c                           | 113 +++++++++++++---------
 net/batman-adv/icmp_socket.c              |   2 +-
 net/batman-adv/log.c                      |   2 +-
 net/rfkill/core.c                         |   2 +-
 sound/core/control.c                      |   2 +-
 sound/core/rawmidi.c                      |   2 +-
 sound/core/seq/seq_clientmgr.c            |   2 +-
 sound/core/timer.c                        |   2 +-
 105 files changed, 179 insertions(+), 158 deletions(-)

-- 
2.20.1

[PATCH 1/3] dtlk: remove double call to nonseekable_open

[Kirill Smelkov]

dtlk_open currently has 2 calls to nonseekable_open which are both
executed on success path. It was not hurting to make the extra call as
nonseekable_open is only changing file->f_flags in idempotent way.
However the first nonseekable_open is indeed both unneeded and looks
suspicious.

The first nonseekable_open was added in 6244f13c51 ("Fix up a couple of
drivers - notable sg - for nonseekability."; 2004-Aug-7). The second
nonseekable_open call was introduced in dc5c724584 ("Remove ESPIPE logic
from drivers, letting the VFS layer handle it instead.; 2004-Aug-8). The
latter patch being mass change probably missed to remove
nonseekable_open that was introduced into dtlk_open the day before.

Fix it: remove the extra/unneeded nonseekable_open call and leave the
call to nonseekable_open only on the path where we are actually opening
the file.

Suggested-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
---
 drivers/char/dtlk.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/char/dtlk.c b/drivers/char/dtlk.c
index f882460b5a44..669c3311adc4 100644
--- a/drivers/char/dtlk.c
+++ b/drivers/char/dtlk.c
@@ -298,7 +298,6 @@ static int dtlk_open(struct inode *inode, struct file *file)
 {
 	TRACE_TEXT("(dtlk_open");
 
-	nonseekable_open(inode, file);
 	switch (iminor(inode)) {
 	case DTLK_MINOR:
 		if (dtlk_busy)
-- 
2.20.1

[PATCH 3/3] vfs: pass ppos=NULL to .read()/.write() of FMODE_STREAM files

[Kirill Smelkov]

This amends commit 10dce8af3422 ("fs: stream_open - opener for
stream-like files so that read and write can run simultaneously without
deadlock") in how position is passed into .read()/.write() handler for
stream-like files:

Rasmus noticed that we currently pass 0 as position and ignore any position
change if that is done by a file implementation. This papers over bugs if ppos
is used in files that declare themselves as being stream-like as such bugs will
go unnoticed. Even if a file implementation is correctly converted into using
stream_open, its read/write later could be changed to use ppos and even though
that won't be working correctly, that bug might go unnoticed without someone
doing wrong behaviour analysis. It is thus better to pass ppos=NULL into
read/write for stream-like files as that don't give any chance for ppos usage
bugs because it will oops if ppos is ever used inside .read() or .write().

Note 1: rw_verify_area, new_sync_{read,write} needs to be updated
because they are called by vfs_read/vfs_write & friends before
file_operations .read/.write .

Note 2: if file backend uses new-style .read_iter/.write_iter, position
is still passed into there as non-pointer kiocb.ki_pos . Currently
stream_open.cocci (semantic patch added by 10dce8af3422) ignores files
whose file_operations has *_iter methods.

Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
---
 fs/open.c       |   5 ++-
 fs/read_write.c | 113 ++++++++++++++++++++++++++++--------------------
 2 files changed, 70 insertions(+), 48 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index a00350018a47..9c7d724a6f67 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1219,8 +1219,9 @@ EXPORT_SYMBOL(nonseekable_open);
 /*
  * stream_open is used by subsystems that want stream-like file descriptors.
  * Such file descriptors are not seekable and don't have notion of position
- * (file.f_pos is always 0). Contrary to file descriptors of other regular
- * files, .read() and .write() can run simultaneously.
+ * (file.f_pos is always 0 and ppos passed to .read()/.write() is always NULL).
+ * Contrary to file descriptors of other regular files, .read() and .write()
+ * can run simultaneously.
  *
  * stream_open never fails and is marked to return int so that it could be
  * directly used as file_operations.open .
diff --git a/fs/read_write.c b/fs/read_write.c
index 61b43ad7608e..c543d965e288 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -365,29 +365,37 @@ SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned long, offset_high,
 int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t count)
 {
 	struct inode *inode;
-	loff_t pos;
 	int retval = -EINVAL;
 
 	inode = file_inode(file);
 	if (unlikely((ssize_t) count < 0))
 		return retval;
-	pos = *ppos;
-	if (unlikely(pos < 0)) {
-		if (!unsigned_offsets(file))
-			return retval;
-		if (count >= -pos) /* both values are in 0..LLONG_MAX */
-			return -EOVERFLOW;
-	} else if (unlikely((loff_t) (pos + count) < 0)) {
-		if (!unsigned_offsets(file))
-			return retval;
-	}
 
-	if (unlikely(inode->i_flctx && mandatory_lock(inode))) {
-		retval = locks_mandatory_area(inode, file, pos, pos + count - 1,
-				read_write == READ ? F_RDLCK : F_WRLCK);
-		if (retval < 0)
-			return retval;
+	/*
+	 * ranged mandatory locking does not apply to streams - it makes sense
+	 * only for files where position has a meaning.
+	 */
+	if (ppos) {
+		loff_t pos = *ppos;
+
+		if (unlikely(pos < 0)) {
+			if (!unsigned_offsets(file))
+				return retval;
+			if (count >= -pos) /* both values are in 0..LLONG_MAX */
+				return -EOVERFLOW;
+		} else if (unlikely((loff_t) (pos + count) < 0)) {
+			if (!unsigned_offsets(file))
+				return retval;
+		}
+
+		if (unlikely(inode->i_flctx && mandatory_lock(inode))) {
+			retval = locks_mandatory_area(inode, file, pos, pos + count - 1,
+					read_write == READ ? F_RDLCK : F_WRLCK);
+			if (retval < 0)
+				return retval;
+		}
 	}
+
 	return security_file_permission(file,
 				read_write == READ ? MAY_READ : MAY_WRITE);
 }
@@ -400,12 +408,13 @@ static ssize_t new_sync_read(struct file *filp, char __user *buf, size_t len, lo
 	ssize_t ret;
 
 	init_sync_kiocb(&kiocb, filp);
-	kiocb.ki_pos = *ppos;
+	kiocb.ki_pos = (ppos ? *ppos : 0);
 	iov_iter_init(&iter, READ, &iov, 1, len);
 
 	ret = call_read_iter(filp, &kiocb, &iter);
 	BUG_ON(ret == -EIOCBQUEUED);
-	*ppos = kiocb.ki_pos;
+	if (ppos)
+		*ppos = kiocb.ki_pos;
 	return ret;
 }
 
@@ -468,12 +477,12 @@ static ssize_t new_sync_write(struct file *filp, const char __user *buf, size_t
 	ssize_t ret;
 
 	init_sync_kiocb(&kiocb, filp);
-	kiocb.ki_pos = *ppos;
+	kiocb.ki_pos = (ppos ? *ppos : 0);
 	iov_iter_init(&iter, WRITE, &iov, 1, len);
 
 	ret = call_write_iter(filp, &kiocb, &iter);
 	BUG_ON(ret == -EIOCBQUEUED);
-	if (ret > 0)
+	if (ret > 0 && ppos)
 		*ppos = kiocb.ki_pos;
 	return ret;
 }
@@ -558,15 +567,10 @@ ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_
 	return ret;
 }
 
-static inline loff_t file_pos_read(struct file *file)
-{
-	return file->f_mode & FMODE_STREAM ? 0 : file->f_pos;
-}
-
-static inline void file_pos_write(struct file *file, loff_t pos)
+/* file_ppos returns &file->f_pos or NULL if file is stream */
+static inline loff_t *file_ppos(struct file *file)
 {
-	if ((file->f_mode & FMODE_STREAM) == 0)
-		file->f_pos = pos;
+	return file->f_mode & FMODE_STREAM ? NULL : &file->f_pos;
 }
 
 ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count)
@@ -575,10 +579,14 @@ ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count)
 	ssize_t ret = -EBADF;
 
 	if (f.file) {
-		loff_t pos = file_pos_read(f.file);
-		ret = vfs_read(f.file, buf, count, &pos);
-		if (ret >= 0)
-			file_pos_write(f.file, pos);
+		loff_t pos, *ppos = file_ppos(f.file);
+		if (ppos) {
+			pos = *ppos;
+			ppos = &pos;
+		}
+		ret = vfs_read(f.file, buf, count, ppos);
+		if (ret >= 0 && ppos)
+			f.file->f_pos = pos;
 		fdput_pos(f);
 	}
 	return ret;
@@ -595,10 +603,14 @@ ssize_t ksys_write(unsigned int fd, const char __user *buf, size_t count)
 	ssize_t ret = -EBADF;
 
 	if (f.file) {
-		loff_t pos = file_pos_read(f.file);
-		ret = vfs_write(f.file, buf, count, &pos);
-		if (ret >= 0)
-			file_pos_write(f.file, pos);
+		loff_t pos, *ppos = file_ppos(f.file);
+		if (ppos) {
+			pos = *ppos;
+			ppos = &pos;
+		}
+		ret = vfs_write(f.file, buf, count, ppos);
+		if (ret >= 0 && ppos)
+			f.file->f_pos = pos;
 		fdput_pos(f);
 	}
 
@@ -673,14 +685,15 @@ static ssize_t do_iter_readv_writev(struct file *filp, struct iov_iter *iter,
 	ret = kiocb_set_rw_flags(&kiocb, flags);
 	if (ret)
 		return ret;
-	kiocb.ki_pos = *ppos;
+	kiocb.ki_pos = (ppos ? *ppos : 0);
 
 	if (type == READ)
 		ret = call_read_iter(filp, &kiocb, iter);
 	else
 		ret = call_write_iter(filp, &kiocb, iter);
 	BUG_ON(ret == -EIOCBQUEUED);
-	*ppos = kiocb.ki_pos;
+	if (ppos)
+		*ppos = kiocb.ki_pos;
 	return ret;
 }
 
@@ -1013,10 +1026,14 @@ static ssize_t do_readv(unsigned long fd, const struct iovec __user *vec,
 	ssize_t ret = -EBADF;
 
 	if (f.file) {
-		loff_t pos = file_pos_read(f.file);
-		ret = vfs_readv(f.file, vec, vlen, &pos, flags);
-		if (ret >= 0)
-			file_pos_write(f.file, pos);
+		loff_t pos, *ppos = file_ppos(f.file);
+		if (ppos) {
+			pos = *ppos;
+			ppos = &pos;
+		}
+		ret = vfs_readv(f.file, vec, vlen, ppos, flags);
+		if (ret >= 0 && ppos)
+			f.file->f_pos = pos;
 		fdput_pos(f);
 	}
 
@@ -1033,10 +1050,14 @@ static ssize_t do_writev(unsigned long fd, const struct iovec __user *vec,
 	ssize_t ret = -EBADF;
 
 	if (f.file) {
-		loff_t pos = file_pos_read(f.file);
-		ret = vfs_writev(f.file, vec, vlen, &pos, flags);
-		if (ret >= 0)
-			file_pos_write(f.file, pos);
+		loff_t pos, *ppos = file_ppos(f.file);
+		if (ppos) {
+			pos = *ppos;
+			ppos = &pos;
+		}
+		ret = vfs_writev(f.file, vec, vlen, ppos, flags);
+		if (ret >= 0 && ppos)
+			f.file->f_pos = pos;
 		fdput_pos(f);
 	}
 
-- 
2.20.1

Re: [PATCH 0/3] stream_open bits for Linux 5.2

[Linus Torvalds]

On Mon, May 6, 2019 at 10:20 AM Kirill Smelkov <kirr@nexedi.com> wrote:
>
> Maybe it will help: the patches can be also pulled from here:
>
>         git pull https://lab.nexedi.com/kirr/linux.git y/stream_open-5.2

I'll take this, but I generally *really* want a signed tag for
non-kernel.org git tree sources. The gpg key used for signing doesn't
necessarily even have to be signed by others yet, but just the fact
that there's a pgp key means that then future pulls at least verify
that it's the sam,e controlling entity, and we can get the signatures
later.

For something one-time where I will then look through the details of
each commit it's not like I absolutely require it, which is why I'm
pulling it, but just in general I wanted to point this out.

                        Linus

Re: [PATCH 0/3] stream_open bits for Linux 5.2

[Kirill Smelkov]

On Tue, May 07, 2019 at 11:54:21AM -0700, Linus Torvalds wrote:
> On Mon, May 6, 2019 at 10:20 AM Kirill Smelkov <kirr@nexedi.com> wrote:
> >
> > Maybe it will help: the patches can be also pulled from here:
> >
> >         git pull https://lab.nexedi.com/kirr/linux.git y/stream_open-5.2
> 
> I'll take this, but I generally *really* want a signed tag for
> non-kernel.org git tree sources. The gpg key used for signing doesn't
> necessarily even have to be signed by others yet, but just the fact
> that there's a pgp key means that then future pulls at least verify
> that it's the sam,e controlling entity, and we can get the signatures
> later.
>
> For something one-time where I will then look through the details of
> each commit it's not like I absolutely require it, which is why I'm
> pulling it, but just in general I wanted to point this out.
> 
>                         Linus

Thanks a lot.

I've pushed corresponding gpg-signed tag (stream_open-5.2) to my tree. I
did not go the gpg way initially because we do not have a gpg-trust
relation established and so I thought that signing was useless.

Just for the record, here is the key that was used to make the
signature: https://pgp.key-server.io/search/kirr@nexedi.com
(fingerprint: 0955B024250EEFFCFE42365B66CA788413F67549)

Kirill

Re: [PATCH 0/3] stream_open bits for Linux 5.2

[Linus Torvalds]

On Tue, May 7, 2019 at 12:09 PM Kirill Smelkov <kirr@nexedi.com> wrote:
>
> I've pushed corresponding gpg-signed tag (stream_open-5.2) to my tree. I
> did not go the gpg way initially because we do not have a gpg-trust
> relation established and so I thought that signing was useless.

Ok, since I hadn't pushed out my pull yet, I just re-did it with your
signature, so that the key is visible in the git tree.

                   Linus

Re: [PATCH 0/3] stream_open bits for Linux 5.2

[Kirill Smelkov]

On Tue, May 07, 2019 at 12:18:03PM -0700, Linus Torvalds wrote:
> On Tue, May 7, 2019 at 12:09 PM Kirill Smelkov <kirr@nexedi.com> wrote:
> >
> > I've pushed corresponding gpg-signed tag (stream_open-5.2) to my tree. I
> > did not go the gpg way initially because we do not have a gpg-trust
> > relation established and so I thought that signing was useless.
> 
> Ok, since I hadn't pushed out my pull yet, I just re-did it with your
> signature, so that the key is visible in the git tree.

Thanks.