[PATCH v2 4/4] splice: fix updating sd->pos wrongly

[PATCH v2 4/4] splice: fix updating sd->pos wrongly

[Changli Gao]

fix updating sd->pos wrongly.

In error path, we don't need to updating sd->pos, if the file isn't seekable.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
 fs/splice.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/splice.c b/fs/splice.c
index 57172ca..c69b241 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1185,7 +1185,8 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
 		sd->pos = pos;
 
 		if (ret < read_len) {
-			sd->pos = prev_pos + ret;
+			if (sd->pos)
+				sd->pos = prev_pos + ret;
 			goto out_release;
 		}
 	}

Re: [PATCH v2 4/4] splice: fix updating sd->pos wrongly

[Miklos Szeredi]

On Wed, 26 May 2010, Changli Gao wrote:
> fix updating sd->pos wrongly.
> 
> In error path, we don't need to updating sd->pos, if the file isn't seekable.

This patch is nonsense.  Why should we handle sd->pos != 0 case
differently?

> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
>  fs/splice.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> diff --git a/fs/splice.c b/fs/splice.c
> index 57172ca..c69b241 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1185,7 +1185,8 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
>  		sd->pos = pos;
>  
>  		if (ret < read_len) {
> -			sd->pos = prev_pos + ret;
> +			if (sd->pos)
> +				sd->pos = prev_pos + ret;
>  			goto out_release;
>  		}
>  	}
> --
> 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: [PATCH v2 4/4] splice: fix updating sd->pos wrongly

[Changli Gao]

On Fri, May 28, 2010 at 5:42 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, 26 May 2010, Changli Gao wrote:
>> fix updating sd->pos wrongly.
>>
>> In error path, we don't need to updating sd->pos, if the file isn't seekable.
>
> This patch is nonsense.  Why should we handle sd->pos != 0 case
> differently?
>

If the in file isn't seekable, its splice_read won't update *ppos, so
in the error path, we'd better not change it too. Otherwise, some
assumption will go wrong.

ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
                        struct pipe_inode_info *pipe, size_t len,
                        unsigned int flags)
{
        struct sock *sk = sock->sk;
        struct tcp_splice_state tss = {
                .pipe = pipe,
                .len = len,
                .flags = flags,
        };
        long timeo;
        ssize_t spliced;
        int ret;

        /*
         * We can't seek on a socket input
         */
        if (unlikely(*ppos))
                return -ESPIPE;


-- 
Regards，
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH v2 4/4] splice: fix updating sd->pos wrongly

[Miklos Szeredi]

On Fri, 28 May 2010, Changli Gao wrote:
> On Fri, May 28, 2010 at 5:42 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > On Wed, 26 May 2010, Changli Gao wrote:
> >> fix updating sd->pos wrongly.
> >>
> >> In error path, we don't need to updating sd->pos, if the file isn't seekable.
> >
> > This patch is nonsense.  Why should we handle sd->pos != 0 case
> > differently?
> >
> 
> If the in file isn't seekable, its splice_read won't update *ppos, so
> in the error path, we'd better not change it too. Otherwise, some
> assumption will go wrong.

That may be true, but the patch is still nonsense.

Look, your patch is updating/not updating sd->pos based on whether it
is zero or not.  It will prevent updating the position for sockets,
but it will also prevent updating the position for regular files if
the position is zero, which is really not what we want.

Since these patches modify the behavior of splice in subtle ways, it
would be good if you had some test program to verify that you aren't
breaking something.

Thanks,
Miklos
--
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: [PATCH v2 4/4] splice: fix updating sd->pos wrongly

[Changli Gao]

On Fri, May 28, 2010 at 6:13 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, 28 May 2010, Changli Gao wrote:
>> On Fri, May 28, 2010 at 5:42 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> > On Wed, 26 May 2010, Changli Gao wrote:
>> >> fix updating sd->pos wrongly.
>> >>
>> >> In error path, we don't need to updating sd->pos, if the file isn't seekable.
>> >
>> > This patch is nonsense.  Why should we handle sd->pos != 0 case
>> > differently?
>> >
>>
>> If the in file isn't seekable, its splice_read won't update *ppos, so
>> in the error path, we'd better not change it too. Otherwise, some
>> assumption will go wrong.
>
> That may be true, but the patch is still nonsense.
>
> Look, your patch is updating/not updating sd->pos based on whether it
> is zero or not.  It will prevent updating the position for sockets,
> but it will also prevent updating the position for regular files if
> the position is zero, which is really not what we want.

I think you misread my patch. Before checking the sd->pos, sd->pos
already is updated with the value returned by splice_read(), so if in
file is seekabble, sd->pos is non-zero when I checking it.



-- 
Regards，
Changli Gao(xiaosuo@gmail.com)
--
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: [PATCH v2 4/4] splice: fix updating sd->pos wrongly

[Miklos Szeredi]

On Fri, 28 May 2010, Changli Gao wrote:
> On Fri, May 28, 2010 at 6:13 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > On Fri, 28 May 2010, Changli Gao wrote:
> >> On Fri, May 28, 2010 at 5:42 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> >> > On Wed, 26 May 2010, Changli Gao wrote:
> >> >> fix updating sd->pos wrongly.
> >> >>
> >> >> In error path, we don't need to updating sd->pos, if the file isn't seekable.
> >> >
> >> > This patch is nonsense.  Why should we handle sd->pos != 0 case
> >> > differently?
> >> >
> >>
> >> If the in file isn't seekable, its splice_read won't update *ppos, so
> >> in the error path, we'd better not change it too. Otherwise, some
> >> assumption will go wrong.
> >
> > That may be true, but the patch is still nonsense.
> >
> > Look, your patch is updating/not updating sd->pos based on whether it
> > is zero or not.  It will prevent updating the position for sockets,
> > but it will also prevent updating the position for regular files if
> > the position is zero, which is really not what we want.
> 
> I think you misread my patch.

Right :)

>  Before checking the sd->pos, sd->pos
> already is updated with the value returned by splice_read(), so if in
> file is seekabble, sd->pos is non-zero when I checking it.

I see the logic now.  But it's really confusing, at least some
comments would be needed.

But I think it would be better not to try reusing sd->pos for reading,
but supplying an "loff_t *read_ppos" to splice_direct_to_actor and
leaving sd->pos alone (for the write side to use and update as
necessary).

Thanks,
Miklos

Re: [PATCH v2 4/4] splice: fix updating sd->pos wrongly

[Changli Gao]

On Fri, May 28, 2010 at 7:11 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, 28 May 2010, Changli Gao wrote:
>>  Before checking the sd->pos, sd->pos
>> already is updated with the value returned by splice_read(), so if in
>> file is seekabble, sd->pos is non-zero when I checking it.
>
> I see the logic now.  But it's really confusing, at least some
> comments would be needed.
>
> But I think it would be better not to try reusing sd->pos for reading,
> but supplying an "loff_t *read_ppos" to splice_direct_to_actor and
> leaving sd->pos alone (for the write side to use and update as
> necessary).
>

               if (ret < read_len) {

I think is much like a hack too. :)

-                       sd->pos = prev_pos + ret;
+                       if (sd->pos)
+                               sd->pos = prev_pos + ret;
                       goto out_release;
               }

Anyway, it can fix the issue I mentioned above. Any future cleanup
should not be mixed in, IMO.

-- 
Regards，
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH v2 4/4] splice: fix updating sd->pos wrongly

[Miklos Szeredi]

On Fri, 28 May 2010, Changli Gao wrote:
> On Fri, May 28, 2010 at 7:11 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > On Fri, 28 May 2010, Changli Gao wrote:
> >>  Before checking the sd->pos, sd->pos
> >> already is updated with the value returned by splice_read(), so if in
> >> file is seekabble, sd->pos is non-zero when I checking it.
> >
> > I see the logic now.  But it's really confusing, at least some
> > comments would be needed.
> >
> > But I think it would be better not to try reusing sd->pos for reading,
> > but supplying an "loff_t *read_ppos" to splice_direct_to_actor and
> > leaving sd->pos alone (for the write side to use and update as
> > necessary).
> >
> 
>                if (ret < read_len) {
> 
> I think is much like a hack too. :)
> 
> -                       sd->pos = prev_pos + ret;
> +                       if (sd->pos)
> +                               sd->pos = prev_pos + ret;
>                        goto out_release;
>                }
> 
> Anyway, it can fix the issue I mentioned above. Any future cleanup
> should not be mixed in, IMO.

Okay, but please add a comment and explaing in the patch header,
because others are going to look a that code and go "WTF???"

Thanks,
Miklos

Re: [PATCH v2 4/4] splice: fix updating sd->pos wrongly

[Jamie Lokier]

Changli Gao wrote:
> On Fri, May 28, 2010 at 6:13 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > On Fri, 28 May 2010, Changli Gao wrote:
> >> On Fri, May 28, 2010 at 5:42 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> >> > On Wed, 26 May 2010, Changli Gao wrote:
> >> >> fix updating sd->pos wrongly.
> >> >>
> >> >> In error path, we don't need to updating sd->pos, if the file isn't seekable.
> >> >
> >> > This patch is nonsense.  Why should we handle sd->pos != 0 case
> >> > differently?
> >> >
> >>
> >> If the in file isn't seekable, its splice_read won't update *ppos, so
> >> in the error path, we'd better not change it too. Otherwise, some
> >> assumption will go wrong.
> >
> > That may be true, but the patch is still nonsense.
> >
> > Look, your patch is updating/not updating sd->pos based on whether it
> > is zero or not.  It will prevent updating the position for sockets,
> > but it will also prevent updating the position for regular files if
> > the position is zero, which is really not what we want.
> 
> I think you misread my patch. Before checking the sd->pos, sd->pos
> already is updated with the value returned by splice_read(), so if in
> file is seekabble, sd->pos is non-zero when I checking it.

Not true if the "file" is /dev/mem or /dev/kmem.  The starting offset
can be negative, so can end at zero.

It's an obscure case and I don't know if you can sendfile from
/dev/{,k}mem anyway :-) but illustrates why sd->pos != 0 is dubious.

-- Jamie
--
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: [PATCH v2 4/4] splice: fix updating sd->pos wrongly

[Changli Gao]

On Sat, May 29, 2010 at 10:09 PM, Jamie Lokier <jamie@shareable.org> wrote:
>
> Not true if the "file" is /dev/mem or /dev/kmem.  The starting offset
> can be negative, so can end at zero.
>
> It's an obscure case and I don't know if you can sendfile from
> /dev/{,k}mem anyway :-) but illustrates why sd->pos != 0 is dubious.
>

So the safe way is checking if splice_read() updates pos or not, isn't it?

if (sd->pos != prev_pos)
    sd->pos = prev_pos + ret;


-- 
Regards，
Changli Gao(xiaosuo@gmail.com)