splice vs O_APPEND

splice vs O_APPEND

[Miklos Szeredi]

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

Re: splice vs O_APPEND

[Linus Torvalds]

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;

Re: splice vs O_APPEND

[Miklos Szeredi]

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

Re: splice vs O_APPEND

[Linus Torvalds]

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;
 

Re: splice vs O_APPEND

[Miklos Szeredi]

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;
 

Re: splice vs O_APPEND

[Linus Torvalds]

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

Re: splice vs O_APPEND

[Miklos Szeredi]

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;

Re: splice vs O_APPEND

[Linus Torvalds]

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;

Re: splice vs O_APPEND

[Jens Axboe]

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

Re: splice vs O_APPEND

[Linus Torvalds]

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

Re: splice vs O_APPEND

[Miklos Szeredi]

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;

Re: splice vs O_APPEND

[Jens Axboe]

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

Re: [stable] splice vs O_APPEND

[Greg KH]

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

Re: [stable] splice vs O_APPEND

[Miklos Szeredi]

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

Re: [stable] splice vs O_APPEND

[Greg KH]

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

Re: splice vs O_APPEND

[Michael Kerrisk]

Hey Linus,

On Thu, Oct 9, 2008 at 11:14 PM, Linus Torvalds
<torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.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-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.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-sUDqSbJrdHQHWmgEVkV9KA@public.gmane.org>
>    Cc: Jens Axboe <ens.axboe-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>    Signed-off-by: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: splice vs O_APPEND

[Michael Kerrisk]

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

Re: splice vs O_APPEND

[Andreas Schwab]

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."

Re: splice vs O_APPEND

[Andreas Schwab]

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."
--
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

Re: splice vs O_APPEND

[Miklos Szeredi]

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

Re: splice vs O_APPEND

[Andreas Schwab]

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."

Re: splice vs O_APPEND

[Andreas Schwab]

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."
--
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