Re: [take 2] Fix for the fundamental network/block layer race in sendfile().

[Eric Dumazet <dada1@cosmosbay.com>]

Evgeniy Polyakov a écrit :
> Hi.
>
> Summary of the previous series with this pompous header:
> when sendfile() returns, pages which it sent can still be queued in tcp
> stack or hardware, so subsequent write into them will endup in
> corrupting data which will be eventually sent. This concerns all
> ->sendpage() users namely sendfile() and splice().
>
> With this patch it is now guaranteed that data was transferred over the
> wire after splice/sendfile returns.
>
>   
1) So a nonblocking operation (O_NDELAY) can become a blocking one now ?

> This approach (besides the fact that previous one was not 100% correct
> dealing with cloned skbs) uses new destructor field in shared info
> structure (introduced by Rusty, afacs they do not collide), which is
> invoked each time data is about to be released (but before actual
> release of data happens).
>   
> Patch works by assigning each page from the sendfile path a couple of
>
>   
> pointers: to page itself to differentiate sendfile pages from all
> others and pointer to pipe structure, which wakes up splice code.
> It is safe to assign the same callback for sendfile and non-sendfile
> pages because of above page pointer - pages which do not have it (slab,
> non-sendfile bio layer and others) will not have pipe structure
> dereferenced from private pointers.
>
> P.S. Previous patch was not an April 1 joke as long as this one :)
>
>   
Hum, I see you forgot me in CC list, dont try to escape :)
> Signed-off-by: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 49a9871..1961a46 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -16,6 +16,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/pagemap.h>
>  #include <linux/splice.h>
> +#include <net/sock.h>
>  #include "read_write.h"
>  
>  #include <asm/uaccess.h>
> @@ -695,6 +696,24 @@ sys_writev(unsigned long fd, const struct iovec __user *vec, unsigned long vlen)
>  	return ret;
>  }
>  
> +static void skb_splice_destructor(struct skb_shared_info *shi)
> +{
> +	if (shi->nr_frags) {
> +		int i;
> +		struct pipe_inode_info *pipe;
> +
> +		for (i = 0; i < shi->nr_frags; i++) {
> +			struct page *page = shi->frags[i].page;
> +
> +			if (page->lru.prev == (struct list_head *)page) {
> +				pipe = (struct pipe_inode_info *)page->lru.next;
>   
    Unless I mistaken, you store in page->lru.next some info to find 
your pipe pointer, assuming it is unique for this page.

What happens if two threads are doing a splice()/sendfile() using the 
same underlying (source) file (and same pages in this file)


> +				if (atomic_dec_return(&pipe->god_blessed_us) == 0)
> +					wake_up(&pipe->wait);
> +			}
> +		}
> +	}
> +}
> +
>  static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>  			   size_t count, loff_t max)
>  {
> @@ -703,6 +722,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>  	loff_t pos;
>  	ssize_t retval;
>  	int fput_needed_in, fput_needed_out, fl;
> +	struct sock *sk;
> +	struct socket *sock;
>  
>  	/*
>  	 * Get input file, and verify that it is ok..
> @@ -762,6 +783,12 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>  		count = max - pos;
>  	}
>  
> +	sock = out_file->private_data;
> +	sk = sock->sk;
> +
> +	sk->sk_user_data = &skb_splice_destructor;
> +	set_bit(SOCK_PRIVATE_CALLBACK, &sock->flags);
> +
>  	fl = 0;
>  #if 0
>  	/*
> @@ -775,6 +802,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
>  #endif
>  	retval = do_splice_direct(in_file, ppos, out_file, count, fl);
>  
> +	clear_bit(SOCK_PRIVATE_CALLBACK, &sock->flags);
> +
>  	if (retval > 0) {
>  		add_rchar(current, retval);
>  		add_wchar(current, retval);
> diff --git a/fs/splice.c b/fs/splice.c
> index 0670c91..dcda32e 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -29,6 +29,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/uio.h>
>  #include <linux/security.h>
> +#include <net/sock.h>
>  
>  /*
>   * Attempt to steal a page from a pipe buffer. This should perhaps go into
> @@ -535,6 +536,8 @@ static int pipe_to_sendpage(struct pipe_inode_info *pipe,
>  	if (!ret) {
>  		more = (sd->flags & SPLICE_F_MORE) || sd->len < sd->total_len;
>  
> +		buf->page->lru.next = (void *)pipe;
>   
is it really allowed here, are you the only user ot this page ?
> +		buf->page->lru.prev = (void *)buf->page;
>  		ret = file->f_op->sendpage(file, buf->page, buf->offset,
>  					   sd->len, &pos, more);
>  	}
> @@ -629,6 +632,9 @@ ssize_t __splice_from_pipe(struct pipe_inode_info *pipe, struct splice_desc *sd,
>  	ret = 0;
>  	do_wakeup = 0;
>