a corner case of open(2)

According to POSIX (and behaviour on other Unices) the following
should succeed: open("/tmp", O_CREAT, 0) does not have O_EXCL and the pathname does refer to existing object, so O_CREAT is ignored and the call is equivalent to open("/tmp", 0), which succeeds.

We have it rejected with EISDIR. The thing is, the standard behaviour
is actually less messy wrt code, and do_last()/lookup_open()/atomic_open() badly needs untangling.

Another place where we produce a bogus EISDIR is O_CREAT|O_EXCL on
an existing directory. POSIX (and other Unices) have EEXIST there. In some cases we produce EEXIST, in some - EISDIR. Uniform EEXIST is actually easier.

It is a change of user-visible behaviour, but I would be very
surprised if anything broke from that change. And it would help to simplify the awful mess we have in there.

Comments?

Existing UNIX behaviour is better. Also, for open() a directory, remember that int fd=open(), fchdir(fd) must work.

Ced

Cedric Blancher <[email protected]>
Institute Pasteur

What's to remember? That part is quite orthogonal to what I'm talking about...

I have to admit that over the past 3 decades of working with Unix-y systems, there's been a number of times I've had to resort to 'od -cx /your/dir/here' to debug issues (/bin/ls -fi is *almost* equivalent, but doesn't show holes in the directory)

The biggest danger I can see is some shell script doing something like:

foobar > $dir/$targetfile

and $targetfile is unset. If we allow a program to get an open fd that refers to a directory, what are the semantics of various operations on that fd?

Huh? We certainly do allow to get an open fd that refers to a directory - how else could ls(1) possibly work? See getdents(2) - it does use an open file descriptor to specify the directory we operate upon.

We also do not allow opening directories for *write*, and in that case EISDIR is the right error (and we do return it). The corner case in question is different:
* O_CREAT present
* O_EXCL absent
* O_RDWR absent
* O_WRONLY absent
* pathname refers to existing directory

That's where POSIX says "just open it for read, as if O_CREAT hadn't been there" and we fail with EISDIR. With both O_CREAT and O_EXCL POSIX says "fail with EEXIST" and we either do that or fail with EISDIR, depending on the pathname details. With either of O_RDWR and O_WRONLY POSIX says "fail with EISDIR, O_CREAT or no O_CREAT" and that's what we do (and would certainly keep doing so).

If you look at the code you'll see
case S_IFDIR:
if (acc_mode & MAY_WRITE)
return -EISDIR;
in may_open() and

        error = -EISDIR;
        if ((open_flag & O_CREAT) && d_is_dir(nd->path.dentry))
                goto out;

in do_last(). The former is "can't open them rw or w", the latter - "can't have O_CREAT on those". With O_CREAT|O_RDWR as in your example either one would trigger (in reality the latter will trigger first and the call of may_open() several lines below won't be reached at all).

Gaah.. I lost a few words in there - /bin/ls is *expecting* to operate on
a directory, so to calls getdents. I meant some generic program that
opened a directory in error, and was expecting to act on "stream of bytes"

OK, that and ftruncate() are about the only ways to cause trouble with a directory opened by accident...

ftruncate() requires the file to be opened for write (which already excludes directories) *and* it requires the file to be a regular one (which is redundant in case of directories, but e.g. a block device can be opened for write and ftruncate would still fail on that). EINVAL in both cases.

truncate() for directories�should fail with EISDIR (see vfs_truncate()); for anything that is neither directory nor regular - EINVAL (same place).

O_TRUNC ends up failing with EISDIR on directories - see

        /* O_TRUNC implies we need access checks for write permissions */
        if (flags & O_TRUNC)
                acc_mode |= MAY_WRITE;

in build_open_flags() and aforementioned bit in may_open(). POSIX is bloody vague on that topic, but that's the common behaviour since 4.3BSD has fixed an fs-corrupting bug in the original implementation (4.2BSD allowed open(directory, O_TRUNC), which both succeeded *and* truncated the damn thing to zero, to great joy of fsck). Note that v7 didn't have O_TRUNC at all - creat(2) was the only way to get it and that opened the sucker r/w, so the usual rules re "no opening directories for write" applied. When O_TRUNC had been introduced, initially they'd missed the possibility of somebody passing it to read-only open() and the need to reject those for directories same as open for write.

Fun bugs caught while trying to massage atomic_open()... Patch below is in vfs.git#for-linus (along with two more fixes); I would like to get an ACK from Miklos on that one - it's his code and this thing had been present in there since the original merge. I might be misreading what it tries to do, but

        open("/mnt/no-such-file", O_CREAT | O_TRUNC);
        perror("open"); errno = 0;
        stat("/mnt/no-such-file", &st);
        perror("stat"); errno = 0;
        open("/mnt/no-such-file", O_CREAT | O_TRUNC);
        perror("open");

should *not* end up with
open: Read-only file system
stat: No such file or directory
open: No such file or directory
no matter what. And it's very easy to arrange just that - mount nfs4 read-only on /mnt and run the snippet above. First open() will fail with EROFS (as it should), but as soon as the thing is in dcache we start getting ENOENT. Obviously bogus.

commit 1aa57f2aaa108ead7d81481af68085b0a77708f1

Author: Al Viro <[email protected]>
Date:   Wed Apr 27 01:11:55 2016 -0400

atomic_open(): fix the handling of create_error

* if we have a hashed negative dentry and either CREAT|EXCL on r/o filesystem, or CREAT|TRUNC on r/o filesystem, or CREAT|EXCL with failing may_o_create(), we should fail with EROFS or the error may_o_create() has returned, but not ENOENT. Which is what the current code ends up returning.

* if we have CREAT|TRUNC hitting a regular file on a read-only filesystem, we can't fail with EROFS here. At the very least, not until we'd done follow_managed() - we might have a writable file (or a device, for that matter) bound on top of that one. Moreover, the code downstream will see that O_TRUNC and attempt to grab the write access (*after* following possible mount), so if we really should fail with EROFS, it will happen. No need to do that inside atomic_open().

The real logics is much simpler than what the current code is trying to do - if we decided to go for simple lookup, ended up with a negative dentry *and* had create_error set, fail with create_error. No matter whether we'd got that negative dentry from lookup_real() or had found it in dcache.

index 1d9ca2d..b458992 100644
@@ -2942,22 +2942,10 @@ no_open:
 		dentry = lookup_real(dir, dentry, nd->flags);
 		if (IS_ERR(dentry))
 			return PTR_ERR(dentry);
-
-		if (create_error) {
-			int open_flag = op->open_flag;
-
-			error = create_error;
-			if ((open_flag & O_EXCL)) {
-				if (!dentry->d_inode)
-					goto out;
-			} else if (!dentry->d_inode) {
-				goto out;
-			} else if ((open_flag & O_TRUNC) &&
-				   d_is_reg(dentry)) {
-				goto out;
-			}
-			/* will fail later, go on to get the right error */
-		}
+	}
+	if (create_error && !dentry->d_inode) {
+		error = create_error;
+		goto out;
 	}
 looked_up:
 	path->dentry = dentry;

Makes perfect sense. Looks like I just fsckd up that part.

Now that Miklos has ACKed that one... The other fix in there is for alignment checks in blk_rq_map_user_iov() - also -stable fodder, even though this one will be a bit more work to backport. Jens, could you ACK that one? This is the patch I'd sent your way several weeks ago (and the one we'd been talking about at LSFMM):

commit 357f435d8a0d32068c75f3c7176434d992b3adb7

Author: Al Viro <[email protected]>
Date:   Fri Apr 8 19:05:19 2016 -0400

fix the copy vs. map logics in blk_rq_map_user_iov()

index a54f054..b9f88b77 100644
@@ -9,24 +9,6 @@
 
 #include "blk.h"
 
-static bool iovec_gap_to_prv(struct request_queue *q,
-			     struct iovec *prv, struct iovec *cur)
-{
-	unsigned long prev_end;
-
-	if (!queue_virt_boundary(q))
-		return false;
-
-	if (prv->iov_base == NULL && prv->iov_len == 0)
-		/* prv is not set - don't check */
-		return false;
-
-	prev_end = (unsigned long)(prv->iov_base + prv->iov_len);
-
-	return (((unsigned long)cur->iov_base & queue_virt_boundary(q)) ||
-		prev_end & queue_virt_boundary(q));
-}
-
 int blk_rq_append_bio(struct request_queue *q, struct request *rq,
 		      struct bio *bio)
 {

@@ -125,31 +107,18 @@ int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
 			struct rq_map_data *map_data,
 			const struct iov_iter *iter, gfp_t gfp_mask)
 {
-	struct iovec iov, prv = {.iov_base = NULL, .iov_len = 0};
-	bool copy = (q->dma_pad_mask & iter->count) || map_data;
+	bool copy = false;
+	unsigned long align = q->dma_pad_mask | queue_dma_alignment(q);
 	struct bio *bio = NULL;
 	struct iov_iter i;
 	int ret;
 
-	if (!iter || !iter->count)
-		return -EINVAL;
-
-	iov_for_each(iov, i, *iter) {
-		unsigned long uaddr = (unsigned long) iov.iov_base;
-
-		if (!iov.iov_len)
-			return -EINVAL;
-
-		/*
-		 * Keep going so we check length of all segments
-		 */
-		if ((uaddr & queue_dma_alignment(q)) ||
-		    iovec_gap_to_prv(q, &prv, &iov))
-			copy = true;
-
-		prv.iov_base = iov.iov_base;
-		prv.iov_len = iov.iov_len;
-	}
+	if (map_data)
+		copy = true;
+	else if (iov_iter_alignment(iter) & align)
+		copy = true;
+	else if (queue_virt_boundary(q))
+		copy = queue_virt_boundary(q) & iov_iter_gap_alignment(iter);
 
 	i = *iter;
 	do {

index fd9bcfe..1b5d1cd 100644
@@ -87,6 +87,7 @@ size_t copy_from_iter(void *addr, size_t bytes, struct iov_iter *i);
 size_t copy_from_iter_nocache(void *addr, size_t bytes, struct iov_iter *i);
 size_t iov_iter_zero(size_t bytes, struct iov_iter *);
 unsigned long iov_iter_alignment(const struct iov_iter *i);
+unsigned long iov_iter_gap_alignment(const struct iov_iter *i);
 void iov_iter_init(struct iov_iter *i, int direction, const struct iovec *iov,
 			unsigned long nr_segs, size_t count);
 void iov_iter_kvec(struct iov_iter *i, int direction, const struct kvec *kvec,

index 5fecddc..ca5316e 100644
@@ -569,6 +569,25 @@ unsigned long iov_iter_alignment(const struct iov_iter *i)
 }
 EXPORT_SYMBOL(iov_iter_alignment);
 
+unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
+{
+        unsigned long res = 0;
+	size_t size = i->count;
+	if (!size)
+		return 0;
+
+	iterate_all_kinds(i, size, v,
+		(res |= (!res ? 0 : (unsigned long)v.iov_base) |
+			(size != v.iov_len ? size : 0), 0),
+		(res |= (!res ? 0 : (unsigned long)v.bv_offset) |
+			(size != v.bv_len ? size : 0)),
+		(res |= (!res ? 0 : (unsigned long)v.iov_base) |
+			(size != v.iov_len ? size : 0))
+		);
+		return res;
+}
+EXPORT_SYMBOL(iov_iter_gap_alignment);
+
 ssize_t iov_iter_get_pages(struct iov_iter *i,
 		   struct page **pages, size_t maxsize, unsigned maxpages,
 		   size_t *start)

Hi!

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html