* splice vs O_APPEND @ 2008-10-09 15:02 Miklos Szeredi 2008-10-09 15:37 ` Linus Torvalds 0 siblings, 1 reply; 19+ messages in thread From: Miklos Szeredi @ 2008-10-09 15:02 UTC (permalink / raw) To: jens.axboe; +Cc: torvalds, linux-kernel, linux-fsdevel While looking at the f_pos corruption thing, I found that splice() to a regular file totally ignores O_APPEND. Which allows users to bypass the append-only restriction. Bad... The only question is how this should be solved? Should splice() respect O_APPEND and ignore the offset? Or should it just return -EINVAL? Thanks, Miklos ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 15:02 splice vs O_APPEND Miklos Szeredi @ 2008-10-09 15:37 ` Linus Torvalds 2008-10-09 16:04 ` Miklos Szeredi 0 siblings, 1 reply; 19+ messages in thread From: Linus Torvalds @ 2008-10-09 15:37 UTC (permalink / raw) To: Miklos Szeredi; +Cc: jens.axboe, linux-kernel, linux-fsdevel On Thu, 9 Oct 2008, Miklos Szeredi wrote: > > While looking at the f_pos corruption thing, I found that splice() to > a regular file totally ignores O_APPEND. Which allows users to bypass > the append-only restriction. Bad... > > The only question is how this should be solved? Should splice() > respect O_APPEND and ignore the offset? Or should it just return > -EINVAL? Good catch. sendfile() has the same issue, but I don't think we ever did sendpage() for any filesystems, so it won't ever be relevant, and this is probably just a splice issue. EINVAL seems the simplest thing. Should check S_IMMUTABLE too for that matter. Possible patch appended. I do wonder if we shouldn't just do this in rw_verify_area(). The whole reason for that function is that we used to have all those flock checks etc spread out all over, and some path would inevitably just miss one check or another. It's kind of stupid to expect low-level filesystems to do the IS_APPEND/IS_IMMUTABLE checks. Comments? Linus --- fs/splice.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/fs/splice.c b/fs/splice.c index 1bbc6f4..769b2d3 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -891,6 +891,7 @@ static long do_splice_from(struct pipe_inode_info *pipe, struct file *out, loff_t *ppos, size_t len, unsigned int flags) { int ret; + struct inode *inode; if (unlikely(!out->f_op || !out->f_op->splice_write)) return -EINVAL; @@ -898,6 +899,12 @@ static long do_splice_from(struct pipe_inode_info *pipe, struct file *out, if (unlikely(!(out->f_mode & FMODE_WRITE))) return -EBADF; + inode = out->f_dentry->d_inode; + if (IS_IMMUTABLE(inode)) + return -EPERM; + if (IS_APPEND(inode)) + return -EINVAL; + ret = rw_verify_area(WRITE, out, ppos, len); if (unlikely(ret < 0)) return ret; ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 15:37 ` Linus Torvalds @ 2008-10-09 16:04 ` Miklos Szeredi 2008-10-09 16:17 ` Linus Torvalds 2008-10-09 16:30 ` Andreas Schwab 0 siblings, 2 replies; 19+ messages in thread From: Miklos Szeredi @ 2008-10-09 16:04 UTC (permalink / raw) To: torvalds; +Cc: miklos, jens.axboe, linux-kernel, linux-fsdevel On Thu, 9 Oct 2008, Linus Torvalds wrote: > I do wonder if we shouldn't just do this in rw_verify_area(). The whole > reason for that function is that we used to have all those flock checks > etc spread out all over, and some path would inevitably just miss one > check or another. It's kind of stupid to expect low-level filesystems to > do the IS_APPEND/IS_IMMUTABLE checks. Do we expect them? I thought we don't care if it's marked immutable or append-only after the file has been opened, same as with normal permissions. > Comments? Your patch still ignores O_APPEND, is that what we want? It sounds sort of strange. pwrite() for example honors O_APPEND and ignores the position, AFAICS. Miklos ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 16:04 ` Miklos Szeredi @ 2008-10-09 16:17 ` Linus Torvalds 2008-10-09 16:30 ` Miklos Szeredi 2008-10-09 16:30 ` Andreas Schwab 1 sibling, 1 reply; 19+ messages in thread From: Linus Torvalds @ 2008-10-09 16:17 UTC (permalink / raw) To: Miklos Szeredi; +Cc: jens.axboe, linux-kernel, linux-fsdevel On Thu, 9 Oct 2008, Miklos Szeredi wrote: > > Your patch still ignores O_APPEND, is that what we want? It sounds > sort of strange. pwrite() for example honors O_APPEND and ignores the > position, AFAICS. You're right. We can (and should) just check O_APPEND, because it must be set if IS_APPEND() is set on the inode. And yeah, IS_IMMUTABLE is checked at open too. So no worries. And it turns out that handling O_APPEND is actually pretty easy, so instead of doing -EINVAL, we can just implement it. Something like this (untested, of course). Does this look better? Linus --- fs/splice.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/fs/splice.c b/fs/splice.c index 1bbc6f4..8aca87b 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -1120,11 +1120,17 @@ static long do_splice(struct file *in, loff_t __user *off_in, if (off_in) return -ESPIPE; if (off_out) { + if (out->f_flags & O_APPEND) + return -EINVAL; if (out->f_op->llseek == no_llseek) return -EINVAL; if (copy_from_user(&offset, off_out, sizeof(loff_t))) return -EFAULT; off = &offset; + } else if (out->f_flags & O_APPEND) { + struct inode *inode = out->f_dentry->d_inode; + offset = i_size_read(inode); + off = &offset; } else off = &out->f_pos; ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 16:17 ` Linus Torvalds @ 2008-10-09 16:30 ` Miklos Szeredi 2008-10-09 19:22 ` Linus Torvalds 0 siblings, 1 reply; 19+ messages in thread From: Miklos Szeredi @ 2008-10-09 16:30 UTC (permalink / raw) To: torvalds; +Cc: miklos, jens.axboe, linux-kernel, linux-fsdevel On Thu, 9 Oct 2008, Linus Torvalds wrote: > And it turns out that handling O_APPEND is actually pretty easy, so > instead of doing -EINVAL, we can just implement it. Something like this > (untested, of course). > > Does this look better? Yeah, only the append is now racy because the O_APPEND check is outside i_mutex. So maybe just stick with -EINVAL in do_splice_from()? That also covers do_splice_direct(), which is used in NFS and sendfile() and a couple of other places. We know that nobody is currently relying on O_APPEND semantics with splice, so this should be OK. Untested patch... Miklos Index: linux-2.6/fs/splice.c =================================================================== --- linux-2.6.orig/fs/splice.c 2008-08-29 14:39:20.000000000 +0200 +++ linux-2.6/fs/splice.c 2008-10-09 18:19:25.000000000 +0200 @@ -892,6 +892,9 @@ static long do_splice_from(struct pipe_i { int ret; + if (out->f_flags & O_APPEND) + return -EINVAL; + if (unlikely(!out->f_op || !out->f_op->splice_write)) return -EINVAL; ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 16:30 ` Miklos Szeredi @ 2008-10-09 19:22 ` Linus Torvalds 2008-10-09 19:51 ` Miklos Szeredi 0 siblings, 1 reply; 19+ messages in thread From: Linus Torvalds @ 2008-10-09 19:22 UTC (permalink / raw) To: Miklos Szeredi; +Cc: jens.axboe, linux-kernel, linux-fsdevel On Thu, 9 Oct 2008, Miklos Szeredi wrote: > > We know that nobody is currently relying on O_APPEND semantics with > splice, so this should be OK. Having now realized that apparently you can't rely on O_APPEND for anything but plain write _anyway_ (ie pwrite shouldn't honor it), I'm going to drop this thing as "let's think about it more". Maybe the right thing to do is to just say that O_APPEND (and IS_APPEND) is really not as reliable as people might expect, and just say that the only thing it affects is a plain write() system call. Of course, I think POSIX is crazy, and we probably _should_ always honor O_APPEND, and returning -EINVAL is the right thing for both pwrite and splice, but this is all a murkier issue than it looked like originally, and any possible "security" implications are dubious in that you cannot really depend on O_APPEND/IS_APPEND anyway. Linus ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 19:22 ` Linus Torvalds @ 2008-10-09 19:51 ` Miklos Szeredi 2008-10-09 21:14 ` Linus Torvalds 0 siblings, 1 reply; 19+ messages in thread From: Miklos Szeredi @ 2008-10-09 19:51 UTC (permalink / raw) To: torvalds; +Cc: miklos, jens.axboe, linux-kernel, linux-fsdevel On Thu, 9 Oct 2008, Linus Torvalds wrote: > Of course, I think POSIX is crazy, and we probably _should_ always honor > O_APPEND, and returning -EINVAL is the right thing for both pwrite and > splice, but this is all a murkier issue than it looked like originally, > and any possible "security" implications are dubious in that you cannot > really depend on O_APPEND/IS_APPEND anyway. The thing is, the append-only attribute is absolutely useless without being able to depend on it. So in that sense I think the IS_APPEND issue is important, and I'm fine with your original proposal for that (except we don't need the IS_IMMUTABLE check). I also agree that the O_APPEND issue is murky and should probably be discussed separately. Thanks, Miklos ---- Subject: splice: disallow random writes for append-only inodes From: Linus Torvalds <torvalds@linux-foundation.org> It was possible to write to a random location in an append-only file using splice. Signed-off-by: Miklos Szeredi <mszeredi@suse.cz> --- fs/splice.c | 5 +++++ 1 file changed, 5 insertions(+) Index: linux-2.6/fs/splice.c =================================================================== --- linux-2.6.orig/fs/splice.c 2008-10-09 21:46:07.000000000 +0200 +++ linux-2.6/fs/splice.c 2008-10-09 21:47:42.000000000 +0200 @@ -891,6 +891,7 @@ static long do_splice_from(struct pipe_i loff_t *ppos, size_t len, unsigned int flags) { int ret; + struct inode *inode; if (unlikely(!out->f_op || !out->f_op->splice_write)) return -EINVAL; @@ -898,6 +899,10 @@ static long do_splice_from(struct pipe_i if (unlikely(!(out->f_mode & FMODE_WRITE))) return -EBADF; + inode = out->f_dentry->d_inode; + if (IS_APPEND(inode)) + return -EINVAL; + ret = rw_verify_area(WRITE, out, ppos, len); if (unlikely(ret < 0)) return ret; ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 19:51 ` Miklos Szeredi @ 2008-10-09 21:14 ` Linus Torvalds 2008-10-09 21:20 ` Jens Axboe ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Linus Torvalds @ 2008-10-09 21:14 UTC (permalink / raw) To: Miklos Szeredi; +Cc: jens.axboe, linux-kernel, linux-fsdevel On Thu, 9 Oct 2008, Miklos Szeredi wrote: > > The thing is, the append-only attribute is absolutely useless without > being able to depend on it. So in that sense I think the IS_APPEND > issue is important, and I'm fine with your original proposal for that > (except we don't need the IS_IMMUTABLE check). Heh. In the meantime, I had grown to hate that more complex patch. So because I do see your point with IS_APPEND (being different from O_APPEND), but because I also think that O_APPEND itself is a gray and murky area, I just committed the following. I doubt anybody will ever even notice it, but while I think it's all debatable, we might as well debate it with this in place. I do agree that it's "safer" behaviour. Linus --- commit a05b4085484ac45558810e4c5928e5a291c20f65 Author: Linus Torvalds <torvalds@linux-foundation.org> Date: Thu Oct 9 14:04:54 2008 -0700 Don't allow splice() to files opened with O_APPEND This is debatable, but while we're debating it, let's disallow the combination of splice and an O_APPEND destination. It's not entirely clear what the semantics of O_APPEND should be, and POSIX apparently expects pwrite() to ignore O_APPEND, for example. So we could make up any semantics we want, including the old ones. But Miklos convinced me that we should at least give it some thought, and that accepting writes at arbitrary offsets is wrong at least for IS_APPEND() files (which always have O_APPEND set, even if the reverse isn't true: you can obviously have O_APPEND set on a regular file). So disallow O_APPEND entirely for now. I doubt anybody cares, and this way we have one less gray area to worry about. Reported-and-argued-for-by: Miklos Szeredi <miklos@szeredi.hu> Cc: Jens Axboe <ens.axboe@oracle.com> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> diff --git a/fs/splice.c b/fs/splice.c index 1bbc6f4..a1e701c 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -898,6 +898,9 @@ static long do_splice_from(struct pipe_inode_info *pipe, struct file *out, if (unlikely(!(out->f_mode & FMODE_WRITE))) return -EBADF; + if (unlikely(out->f_flags & O_APPEND)) + return -EINVAL; + ret = rw_verify_area(WRITE, out, ppos, len); if (unlikely(ret < 0)) return ret; ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 21:14 ` Linus Torvalds @ 2008-10-09 21:20 ` Jens Axboe 2008-10-09 21:27 ` Linus Torvalds 2008-10-10 9:46 ` Miklos Szeredi 2008-10-10 10:23 ` Michael Kerrisk 2 siblings, 1 reply; 19+ messages in thread From: Jens Axboe @ 2008-10-09 21:20 UTC (permalink / raw) To: Linus Torvalds; +Cc: Miklos Szeredi, linux-kernel, linux-fsdevel On Thu, Oct 09 2008, Linus Torvalds wrote: > > > On Thu, 9 Oct 2008, Miklos Szeredi wrote: > > > > The thing is, the append-only attribute is absolutely useless without > > being able to depend on it. So in that sense I think the IS_APPEND > > issue is important, and I'm fine with your original proposal for that > > (except we don't need the IS_IMMUTABLE check). > > Heh. In the meantime, I had grown to hate that more complex patch. > > So because I do see your point with IS_APPEND (being different from > O_APPEND), but because I also think that O_APPEND itself is a gray and > murky area, I just committed the following. I doubt anybody will ever even > notice it, but while I think it's all debatable, we might as well debate > it with this in place. I do agree that it's "safer" behaviour. > > Linus > > --- > commit a05b4085484ac45558810e4c5928e5a291c20f65 > Author: Linus Torvalds <torvalds@linux-foundation.org> > Date: Thu Oct 9 14:04:54 2008 -0700 > > Don't allow splice() to files opened with O_APPEND > > This is debatable, but while we're debating it, let's disallow the > combination of splice and an O_APPEND destination. > > It's not entirely clear what the semantics of O_APPEND should be, and > POSIX apparently expects pwrite() to ignore O_APPEND, for example. So > we could make up any semantics we want, including the old ones. > > But Miklos convinced me that we should at least give it some thought, > and that accepting writes at arbitrary offsets is wrong at least for > IS_APPEND() files (which always have O_APPEND set, even if the reverse > isn't true: you can obviously have O_APPEND set on a regular file). > > So disallow O_APPEND entirely for now. I doubt anybody cares, and this > way we have one less gray area to worry about. > > Reported-and-argued-for-by: Miklos Szeredi <miklos@szeredi.hu> > Cc: Jens Axboe <ens.axboe@oracle.com> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> A little late I see, but FWIW I agree. I doubt that anyone uses splice with O_APPEND anyways, so should be zero-impact on that account at least. > > diff --git a/fs/splice.c b/fs/splice.c > index 1bbc6f4..a1e701c 100644 > --- a/fs/splice.c > +++ b/fs/splice.c > @@ -898,6 +898,9 @@ static long do_splice_from(struct pipe_inode_info *pipe, struct file *out, > if (unlikely(!(out->f_mode & FMODE_WRITE))) > return -EBADF; > > + if (unlikely(out->f_flags & O_APPEND)) > + return -EINVAL; > + > ret = rw_verify_area(WRITE, out, ppos, len); > if (unlikely(ret < 0)) > return ret; -- Jens Axboe ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 21:20 ` Jens Axboe @ 2008-10-09 21:27 ` Linus Torvalds 0 siblings, 0 replies; 19+ messages in thread From: Linus Torvalds @ 2008-10-09 21:27 UTC (permalink / raw) To: Jens Axboe; +Cc: Miklos Szeredi, linux-kernel, linux-fsdevel On Thu, 9 Oct 2008, Jens Axboe wrote: > > A little late I see, but FWIW I agree. Well, not too late, since I hadn't pushed it out. I amended the commit to have an acked-by from you. And now it _is_ pushed out. Linus ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 21:14 ` Linus Torvalds 2008-10-09 21:20 ` Jens Axboe @ 2008-10-10 9:46 ` Miklos Szeredi 2008-10-10 10:06 ` Jens Axboe 2008-10-10 15:49 ` [stable] " Greg KH 2008-10-10 10:23 ` Michael Kerrisk 2 siblings, 2 replies; 19+ messages in thread From: Miklos Szeredi @ 2008-10-10 9:46 UTC (permalink / raw) To: torvalds; +Cc: stable, miklos, jens.axboe, linux-kernel, linux-fsdevel On Thu, 9 Oct 2008, Linus Torvalds wrote: > On Thu, 9 Oct 2008, Miklos Szeredi wrote: > > > > The thing is, the append-only attribute is absolutely useless without > > being able to depend on it. So in that sense I think the IS_APPEND > > issue is important, and I'm fine with your original proposal for that > > (except we don't need the IS_IMMUTABLE check). > > Heh. In the meantime, I had grown to hate that more complex patch. > > So because I do see your point with IS_APPEND (being different from > O_APPEND), but because I also think that O_APPEND itself is a gray and > murky area, I just committed the following. I doubt anybody will ever even > notice it, but while I think it's all debatable, we might as well debate > it with this in place. I do agree that it's "safer" behaviour. Thanks. I suspect this qualifies for stable kernels too. Stable team, can you please add this to your queue? The final commit is: commit efc968d450e013049a662d22727cf132618dcb2f Author: Linus Torvalds <torvalds@linux-foundation.org> Date: Thu Oct 9 14:04:54 2008 -0700 Don't allow splice() to files opened with O_APPEND This is debatable, but while we're debating it, let's disallow the combination of splice and an O_APPEND destination. It's not entirely clear what the semantics of O_APPEND should be, and POSIX apparently expects pwrite() to ignore O_APPEND, for example. So we could make up any semantics we want, including the old ones. But Miklos convinced me that we should at least give it some thought, and that accepting writes at arbitrary offsets is wrong at least for IS_APPEND() files (which always have O_APPEND set, even if the reverse isn't true: you can obviously have O_APPEND set on a regular file). So disallow O_APPEND entirely for now. I doubt anybody cares, and this way we have one less gray area to worry about. Reported-and-argued-for-by: Miklos Szeredi <miklos@szeredi.hu> Acked-by: Jens Axboe <ens.axboe@oracle.com> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> diff --git a/fs/splice.c b/fs/splice.c index 1bbc6f4..a1e701c 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -898,6 +898,9 @@ static long do_splice_from(struct pipe_inode_info *pipe, struct file *out, if (unlikely(!(out->f_mode & FMODE_WRITE))) return -EBADF; + if (unlikely(out->f_flags & O_APPEND)) + return -EINVAL; + ret = rw_verify_area(WRITE, out, ppos, len); if (unlikely(ret < 0)) return ret; ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-10 9:46 ` Miklos Szeredi @ 2008-10-10 10:06 ` Jens Axboe 2008-10-10 15:49 ` [stable] " Greg KH 1 sibling, 0 replies; 19+ messages in thread From: Jens Axboe @ 2008-10-10 10:06 UTC (permalink / raw) To: Miklos Szeredi; +Cc: torvalds, stable, linux-kernel, linux-fsdevel On Fri, Oct 10 2008, Miklos Szeredi wrote: > On Thu, 9 Oct 2008, Linus Torvalds wrote: > > On Thu, 9 Oct 2008, Miklos Szeredi wrote: > > > > > > The thing is, the append-only attribute is absolutely useless without > > > being able to depend on it. So in that sense I think the IS_APPEND > > > issue is important, and I'm fine with your original proposal for that > > > (except we don't need the IS_IMMUTABLE check). > > > > Heh. In the meantime, I had grown to hate that more complex patch. > > > > So because I do see your point with IS_APPEND (being different from > > O_APPEND), but because I also think that O_APPEND itself is a gray and > > murky area, I just committed the following. I doubt anybody will ever even > > notice it, but while I think it's all debatable, we might as well debate > > it with this in place. I do agree that it's "safer" behaviour. > > Thanks. > > I suspect this qualifies for stable kernels too. Stable team, can you > please add this to your queue? > > The final commit is: > > commit efc968d450e013049a662d22727cf132618dcb2f > Author: Linus Torvalds <torvalds@linux-foundation.org> > Date: Thu Oct 9 14:04:54 2008 -0700 > > Don't allow splice() to files opened with O_APPEND > > This is debatable, but while we're debating it, let's disallow the > combination of splice and an O_APPEND destination. > > It's not entirely clear what the semantics of O_APPEND should be, and > POSIX apparently expects pwrite() to ignore O_APPEND, for example. So > we could make up any semantics we want, including the old ones. > > But Miklos convinced me that we should at least give it some thought, > and that accepting writes at arbitrary offsets is wrong at least for > IS_APPEND() files (which always have O_APPEND set, even if the reverse > isn't true: you can obviously have O_APPEND set on a regular file). > > So disallow O_APPEND entirely for now. I doubt anybody cares, and this > way we have one less gray area to worry about. > > Reported-and-argued-for-by: Miklos Szeredi <miklos@szeredi.hu> > Acked-by: Jens Axboe <ens.axboe@oracle.com> And lets then change this to <jens.axboe@oracle.com> :-) > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > diff --git a/fs/splice.c b/fs/splice.c > index 1bbc6f4..a1e701c 100644 > --- a/fs/splice.c > +++ b/fs/splice.c > @@ -898,6 +898,9 @@ static long do_splice_from(struct pipe_inode_info *pipe, struct file *out, > if (unlikely(!(out->f_mode & FMODE_WRITE))) > return -EBADF; > > + if (unlikely(out->f_flags & O_APPEND)) > + return -EINVAL; > + > ret = rw_verify_area(WRITE, out, ppos, len); > if (unlikely(ret < 0)) > return ret; -- Jens Axboe ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [stable] splice vs O_APPEND 2008-10-10 9:46 ` Miklos Szeredi 2008-10-10 10:06 ` Jens Axboe @ 2008-10-10 15:49 ` Greg KH 2008-10-10 16:05 ` Miklos Szeredi 1 sibling, 1 reply; 19+ messages in thread From: Greg KH @ 2008-10-10 15:49 UTC (permalink / raw) To: Miklos Szeredi; +Cc: torvalds, linux-fsdevel, linux-kernel, jens.axboe, stable On Fri, Oct 10, 2008 at 11:46:00AM +0200, Miklos Szeredi wrote: > On Thu, 9 Oct 2008, Linus Torvalds wrote: > > On Thu, 9 Oct 2008, Miklos Szeredi wrote: > > > > > > The thing is, the append-only attribute is absolutely useless without > > > being able to depend on it. So in that sense I think the IS_APPEND > > > issue is important, and I'm fine with your original proposal for that > > > (except we don't need the IS_IMMUTABLE check). > > > > Heh. In the meantime, I had grown to hate that more complex patch. > > > > So because I do see your point with IS_APPEND (being different from > > O_APPEND), but because I also think that O_APPEND itself is a gray and > > murky area, I just committed the following. I doubt anybody will ever even > > notice it, but while I think it's all debatable, we might as well debate > > it with this in place. I do agree that it's "safer" behaviour. > > Thanks. > > I suspect this qualifies for stable kernels too. Stable team, can you > please add this to your queue? Queue for which kernel releases? .25, .26, and/or .27? thanks, greg k-h ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [stable] splice vs O_APPEND 2008-10-10 15:49 ` [stable] " Greg KH @ 2008-10-10 16:05 ` Miklos Szeredi 2008-10-10 16:20 ` Greg KH 0 siblings, 1 reply; 19+ messages in thread From: Miklos Szeredi @ 2008-10-10 16:05 UTC (permalink / raw) To: greg; +Cc: miklos, torvalds, linux-fsdevel, linux-kernel, jens.axboe, stable On Fri, 10 Oct 2008, Greg KH wrote: > On Fri, Oct 10, 2008 at 11:46:00AM +0200, Miklos Szeredi wrote: > > I suspect this qualifies for stable kernels too. Stable team, can you > > please add this to your queue? > > Queue for which kernel releases? .25, .26, and/or .27? 25 and 26. It's in .27. Thanks, Miklos ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [stable] splice vs O_APPEND 2008-10-10 16:05 ` Miklos Szeredi @ 2008-10-10 16:20 ` Greg KH 0 siblings, 0 replies; 19+ messages in thread From: Greg KH @ 2008-10-10 16:20 UTC (permalink / raw) To: Miklos Szeredi; +Cc: torvalds, linux-fsdevel, linux-kernel, jens.axboe, stable On Fri, Oct 10, 2008 at 06:05:48PM +0200, Miklos Szeredi wrote: > On Fri, 10 Oct 2008, Greg KH wrote: > > On Fri, Oct 10, 2008 at 11:46:00AM +0200, Miklos Szeredi wrote: > > > I suspect this qualifies for stable kernels too. Stable team, can you > > > please add this to your queue? > > > > Queue for which kernel releases? .25, .26, and/or .27? > > 25 and 26. It's in .27. Ah, thanks, will queue it up. greg k-h ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 21:14 ` Linus Torvalds 2008-10-09 21:20 ` Jens Axboe 2008-10-10 9:46 ` Miklos Szeredi @ 2008-10-10 10:23 ` Michael Kerrisk 2 siblings, 0 replies; 19+ messages in thread From: Michael Kerrisk @ 2008-10-10 10:23 UTC (permalink / raw) To: Linus Torvalds Cc: Miklos Szeredi, jens.axboe, linux-kernel, linux-fsdevel, linux-api, mtk.manpages Hey Linus, On Thu, Oct 9, 2008 at 11:14 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > On Thu, 9 Oct 2008, Miklos Szeredi wrote: >> >> The thing is, the append-only attribute is absolutely useless without >> being able to depend on it. So in that sense I think the IS_APPEND >> issue is important, and I'm fine with your original proposal for that >> (except we don't need the IS_IMMUTABLE check). > > Heh. In the meantime, I had grown to hate that more complex patch. > > So because I do see your point with IS_APPEND (being different from > O_APPEND), but because I also think that O_APPEND itself is a gray and > murky area, I just committed the following. I doubt anybody will ever even > notice it, but while I think it's all debatable, we might as well debate > it with this in place. I do agree that it's "safer" behaviour. Please do CC linux-api@vger (and if you are kind, me, in case something needs to go into man-pages) on interface changes. Cheers, Michael > --- > commit a05b4085484ac45558810e4c5928e5a291c20f65 > Author: Linus Torvalds <torvalds@linux-foundation.org> > Date: Thu Oct 9 14:04:54 2008 -0700 > > Don't allow splice() to files opened with O_APPEND > > This is debatable, but while we're debating it, let's disallow the > combination of splice and an O_APPEND destination. > > It's not entirely clear what the semantics of O_APPEND should be, and > POSIX apparently expects pwrite() to ignore O_APPEND, for example. So > we could make up any semantics we want, including the old ones. > > But Miklos convinced me that we should at least give it some thought, > and that accepting writes at arbitrary offsets is wrong at least for > IS_APPEND() files (which always have O_APPEND set, even if the reverse > isn't true: you can obviously have O_APPEND set on a regular file). > > So disallow O_APPEND entirely for now. I doubt anybody cares, and this > way we have one less gray area to worry about. > > Reported-and-argued-for-by: Miklos Szeredi <miklos@szeredi.hu> > Cc: Jens Axboe <ens.axboe@oracle.com> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > diff --git a/fs/splice.c b/fs/splice.c > index 1bbc6f4..a1e701c 100644 > --- a/fs/splice.c > +++ b/fs/splice.c > @@ -898,6 +898,9 @@ static long do_splice_from(struct pipe_inode_info *pipe, struct file *out, > if (unlikely(!(out->f_mode & FMODE_WRITE))) > return -EBADF; > > + if (unlikely(out->f_flags & O_APPEND)) > + return -EINVAL; > + > ret = rw_verify_area(WRITE, out, ppos, len); > if (unlikely(ret < 0)) > return ret; > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Found a documentation bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 16:04 ` Miklos Szeredi 2008-10-09 16:17 ` Linus Torvalds @ 2008-10-09 16:30 ` Andreas Schwab 2008-10-09 16:43 ` Miklos Szeredi 1 sibling, 1 reply; 19+ messages in thread From: Andreas Schwab @ 2008-10-09 16:30 UTC (permalink / raw) To: Miklos Szeredi; +Cc: torvalds, jens.axboe, linux-kernel, linux-fsdevel Miklos Szeredi <miklos@szeredi.hu> writes: > pwrite() for example honors O_APPEND and ignores the position, AFAICS. If it does, then it violates POSIX (see <http://www.opengroup.org/austin/docs/austin_325.txt>, where the behavior of pwrite with O_APPEND is clarified). Andreas. -- Andreas Schwab, SuSE Labs, schwab@suse.de SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 16:30 ` Andreas Schwab @ 2008-10-09 16:43 ` Miklos Szeredi 2008-10-09 17:03 ` Andreas Schwab 0 siblings, 1 reply; 19+ messages in thread From: Miklos Szeredi @ 2008-10-09 16:43 UTC (permalink / raw) To: schwab; +Cc: miklos, torvalds, jens.axboe, linux-kernel, linux-fsdevel On Thu, 09 Oct 2008, Andreas Schwab wrote: > Miklos Szeredi <miklos@szeredi.hu> writes: > > > pwrite() for example honors O_APPEND and ignores the position, AFAICS. > > If it does, then it violates POSIX (see I checked and it does. Nobody seems to care though, not even the POSIX conformance checkers :) Miklos ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: splice vs O_APPEND 2008-10-09 16:43 ` Miklos Szeredi @ 2008-10-09 17:03 ` Andreas Schwab 0 siblings, 0 replies; 19+ messages in thread From: Andreas Schwab @ 2008-10-09 17:03 UTC (permalink / raw) To: Miklos Szeredi; +Cc: torvalds, jens.axboe, linux-kernel, linux-fsdevel Miklos Szeredi <miklos@szeredi.hu> writes: > I checked and it does. Nobody seems to care though, not even the > POSIX conformance checkers :) According to <http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6655660> they do now. Andreas. -- Andreas Schwab, SuSE Labs, schwab@suse.de SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2008-10-10 16:54 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-10-09 15:02 splice vs O_APPEND Miklos Szeredi 2008-10-09 15:37 ` Linus Torvalds 2008-10-09 16:04 ` Miklos Szeredi 2008-10-09 16:17 ` Linus Torvalds 2008-10-09 16:30 ` Miklos Szeredi 2008-10-09 19:22 ` Linus Torvalds 2008-10-09 19:51 ` Miklos Szeredi 2008-10-09 21:14 ` Linus Torvalds 2008-10-09 21:20 ` Jens Axboe 2008-10-09 21:27 ` Linus Torvalds 2008-10-10 9:46 ` Miklos Szeredi 2008-10-10 10:06 ` Jens Axboe 2008-10-10 15:49 ` [stable] " Greg KH 2008-10-10 16:05 ` Miklos Szeredi 2008-10-10 16:20 ` Greg KH 2008-10-10 10:23 ` Michael Kerrisk 2008-10-09 16:30 ` Andreas Schwab 2008-10-09 16:43 ` Miklos Szeredi 2008-10-09 17:03 ` Andreas Schwab
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox