tcp_sendpage(): fix broken page iteration
Subsystems (3)
PATCH 1/1 tcp_sendpage(): fix broken page iteration
do_tcp_sendpages() should not iterate 'pages' as an array since it is not an array of 'struct page *', but a pointer to a single entity of 'struct page *' passed on the stack as a parameter to tcp_send_page() (hence it would crash if poffset + psize > PAGE_SIZE, because pages[1] and beyond most probably not constitutes a valid 'struct page *').
Since 'page' points to an array of 'struct page', the obvious fix is to iterate that array instead, and that's what the function should have done in the first place.
Applies to 2.6.21-rc4 and above.
net/ipv4/tcp.c +3 -3
@@ -501,7 +501,7 @@ static inline void tcp_push(struct sock *sk, struct tcp_sock *tp, int flags, } } -static ssize_t do_tcp_sendpages (struct sock *sk,struct page **pages,int poffset, +static ssize_t do_tcp_sendpages (struct sock *sk,struct page *pages,int poffset,size_t psize,int flags) {struct tcp_sock *tp =tcp_sk (sk);
@@ -527,7 +527,7 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page **pages, int poffsewhile (psize >0 ) {struct sk_buff *skb = sk->sk_write_queue.prev; -struct page *page = pages[poffset /PAGE_SIZE ]; +struct page *page = &pages[poffset /PAGE_SIZE ];int copy, i, can_coalesce;int offset = poffset %PAGE_SIZE ;int size =min_t (size_t, psize,PAGE_SIZE - offset);
@@ -630,7 +630,7 @@ ssize_t tcp_sendpage(struct socket *sock, struct page *page, int offset,lock_sock (sk);TCP_CHECK_TIMER (sk); - res =do_tcp_sendpages (sk, &page, offset, size, flags); + res =do_tcp_sendpages (sk, page, offset, size, flags);TCP_CHECK_TIMER (sk);release_sock (sk);return res;
Signature · 3 lines
Dan Aloni XIV LTD, http://www.xivstorage.com da-x (at) monatomic.org, dan (at) xiv.co.il
Quoted 7 lines · Dan Aloni
do_tcp_sendpages()should not iterate 'pages' as an array since it is not an array of 'struct page *', but a pointer to a single entity of 'struct page *' passed on the stack as a parameter totcp_send_page()(hence it would crash if poffset + psize >PAGE_SIZE, because pages[1] and beyond most probably not constitutes a valid 'struct page *').
Quoted 5 lines
Since 'page' points to an array of 'struct page', the obvious fix is to iterate that array instead, and that's what the function should have done in the first place.
Applies to 2.6.21-rc4 and above.
Oops, forgot the obligtory signed-off:
net/ipv4/tcp.c +3 -3
@@ -501,7 +501,7 @@ static inline void tcp_push(struct sock *sk, struct tcp_sock *tp, int flags, } } -static ssize_t do_tcp_sendpages (struct sock *sk,struct page **pages,int poffset, +static ssize_t do_tcp_sendpages (struct sock *sk,struct page *pages,int poffset,size_t psize,int flags) {struct tcp_sock *tp =tcp_sk (sk);
@@ -527,7 +527,7 @@ static ssize_t do_tcp_sendpages(struct sock *sk, struct page **pages, int poffsewhile (psize >0 ) {struct sk_buff *skb = sk->sk_write_queue.prev; -struct page *page = pages[poffset /PAGE_SIZE ]; +struct page *page = &pages[poffset /PAGE_SIZE ];int copy, i, can_coalesce;int offset = poffset %PAGE_SIZE ;int size =min_t (size_t, psize,PAGE_SIZE - offset);
@@ -630,7 +630,7 @@ ssize_t tcp_sendpage(struct socket *sock, struct page *page, int offset,lock_sock (sk);TCP_CHECK_TIMER (sk); - res =do_tcp_sendpages (sk, &page, offset, size, flags); + res =do_tcp_sendpages (sk, page, offset, size, flags);TCP_CHECK_TIMER (sk);release_sock (sk);return res;
Signature · 3 lines
Dan Aloni XIV LTD, http://www.xivstorage.com da-x (at) monatomic.org, dan (at) xiv.co.il
David Miller
maintainerFrom: Dan Aloni <[email protected]>
Date: Sun, 18 Mar 2007 14:43:46 +0200
Quoted 6 lines
do_tcp_sendpages()should not iterate 'pages' as an array since it is not an array of 'struct page *', but a pointer to a single entity of 'struct page *' passed on the stack as a parameter totcp_send_page()(hence it would crash if poffset + psize >PAGE_SIZE, because pages[1] and beyond most probably not constitutes a valid 'struct page *').
do_tcp_sendpages() should never get passed poffset+psize>PAGE_SIZE, that would be a bug.
Feel free to add a BUG() check for that if you wish, and a fix for any caller which violates this.
The code is perfectly fine as-is. It was originally written to accept page arrays, but once it was decided that ->sendpage() would only pass in one page, we simply modified to caller of do_tcp_sendpages() to accomodate this argument passing change, instead of changing do_tcp_sendpages() which is totally unnecessary.
Quoted 10 lines · David Miller
From: Dan Aloni <[email protected]>
Date: Sun, 18 Mar 2007 14:43:46 +0200Quoted 6 lines
do_tcp_sendpages()should not iterate 'pages' as an array since it is not an array of 'struct page *', but a pointer to a single entity of 'struct page *' passed on the stack as a parameter totcp_send_page()(hence it would crash if poffset + psize >PAGE_SIZE, because pages[1] and beyond most probably not constitutes a valid 'struct page *').
Quoted 2 lines
do_tcp_sendpages()should never get passed poffset+psize>PAGE_SIZE, that would be a bug.
Oh, then the name of that function was quite misleading...
Anyway, I thought it would make a valid case for a situation where you have a kmalloc'ed buffer that happens to cross a page boundery and you want to call ->sendpage() to send it over using network DMA.
As I see it, with this constraint you either call sendpage twice or you use kernel_sendmsg(), I am not sure which would me more efficient - I guess it depends on psize. I wish there was a better interface than sendpage that would have factored it in...
Thanks anyway for the heads up.
Signature · 3 lines
Dan Aloni XIV LTD, http://www.xivstorage.com da-x (at) monatomic.org, dan (at) xiv.co.il