The original problem was from nvme-over-tcp code, who mistakenly uses kernel_sendpage() to send pages allocated by __get_free_pages() without __GFP_COMP flag. Such pages don't have refcount (page_count is 0) on tail pages, sending them by kernel_sendpage() may trigger a kernel panic from a corrupted kernel heap, because these pages are incorrectly freed in network stack as page_count 0 pages.
This patch introduces a helper sendpage_ok(), it returns true if the checking page, - is not slab page: PageSlab(page) is false. - has page refcount: page_count(page) is not zero
All drivers who want to send page to remote end by kernel_sendpage() may use this helper to check whether the page is OK. If the helper does not return true, the driver should try other non sendpage method (e.g. sock_no_sendpage()) to handle the page.
Signed-off-by: Coly Li colyli@suse.de Cc: Chaitanya Kulkarni chaitanya.kulkarni@wdc.com Cc: Christoph Hellwig hch@lst.de Cc: Hannes Reinecke hare@suse.de Cc: Jan Kara jack@suse.com Cc: Jens Axboe axboe@kernel.dk Cc: Mikhail Skorzhinskii mskorzhinskiy@solarflare.com Cc: Philipp Reisner philipp.reisner@linbit.com Cc: Sagi Grimberg sagi@grimberg.me Cc: Vlastimil Babka vbabka@suse.com Cc: stable@vger.kernel.org --- include/linux/net.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/include/linux/net.h b/include/linux/net.h index d48ff1180879..05db8690f67e 100644 --- a/include/linux/net.h +++ b/include/linux/net.h @@ -21,6 +21,7 @@ #include <linux/rcupdate.h> #include <linux/once.h> #include <linux/fs.h> +#include <linux/mm.h> #include <linux/sockptr.h>
#include <uapi/linux/net.h> @@ -286,6 +287,21 @@ do { \ #define net_get_random_once_wait(buf, nbytes) \ get_random_once_wait((buf), (nbytes))
+/* + * E.g. XFS meta- & log-data is in slab pages, or bcache meta + * data pages, or other high order pages allocated by + * __get_free_pages() without __GFP_COMP, which have a page_count + * of 0 and/or have PageSlab() set. We cannot use send_page for + * those, as that does get_page(); put_page(); and would cause + * either a VM_BUG directly, or __page_cache_release a page that + * would actually still be referenced by someone, leading to some + * obscure delayed Oops somewhere else. + */ +static inline bool sendpage_ok(struct page *page) +{ + return !PageSlab(page) && page_count(page) >= 1; +} + int kernel_sendmsg(struct socket *sock, struct msghdr *msg, struct kvec *vec, size_t num, size_t len); int kernel_sendmsg_locked(struct sock *sk, struct msghdr *msg,
I think we should go for something simple like this instead:
---
From 4867e158ee86ebd801b4c267e8f8a4a762a71343 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig hch@lst.de Date: Tue, 18 Aug 2020 18:19:23 +0200 Subject: net: bypass ->sendpage for slab pages
Sending Slab or tail pages into ->sendpage will cause really strange delayed oops. Prevent it right in the networking code instead of requiring drivers to work around the fact.
Signed-off-by: Christoph Hellwig hch@lst.de --- net/socket.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/socket.c b/net/socket.c index dbbe8ea7d395da..fbc82eb96d18ce 100644 --- a/net/socket.c +++ b/net/socket.c @@ -3638,7 +3638,12 @@ EXPORT_SYMBOL(kernel_getpeername); int kernel_sendpage(struct socket *sock, struct page *page, int offset, size_t size, int flags) { - if (sock->ops->sendpage) + /* + * sendpage does manipulates the refcount of the passed in page, which + * does not work for Slab pages, or for tails of non-__GFP_COMP + * high order pages. + */ + if (sock->ops->sendpage && !PageSlab(page) && page_count(page) > 0) return sock->ops->sendpage(sock, page, offset, size, flags);
return sock_no_sendpage(sock, page, offset, size, flags);
On 2020/8/19 00:24, Christoph Hellwig wrote:
I think we should go for something simple like this instead:
This idea is fine to me. Should a warning message be through here? IMHO the driver still sends an improper page in, fix it in silence is too kind or over nice to the buggy driver(s).
And maybe the fix in nvme-tcp driver and do_tcp_sendpages() are still necessary. I am not network expert, this is my opinion for reference.
Coly Li
From 4867e158ee86ebd801b4c267e8f8a4a762a71343 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig hch@lst.de Date: Tue, 18 Aug 2020 18:19:23 +0200 Subject: net: bypass ->sendpage for slab pages
Sending Slab or tail pages into ->sendpage will cause really strange delayed oops. Prevent it right in the networking code instead of requiring drivers to work around the fact.
Signed-off-by: Christoph Hellwig hch@lst.de
net/socket.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/socket.c b/net/socket.c index dbbe8ea7d395da..fbc82eb96d18ce 100644 --- a/net/socket.c +++ b/net/socket.c @@ -3638,7 +3638,12 @@ EXPORT_SYMBOL(kernel_getpeername); int kernel_sendpage(struct socket *sock, struct page *page, int offset, size_t size, int flags) {
- if (sock->ops->sendpage)
- /*
* sendpage does manipulates the refcount of the passed in page, which* does not work for Slab pages, or for tails of non-__GFP_COMP* high order pages.*/- if (sock->ops->sendpage && !PageSlab(page) && page_count(page) > 0) return sock->ops->sendpage(sock, page, offset, size, flags);
return sock_no_sendpage(sock, page, offset, size, flags);
On Wed, Aug 19, 2020 at 12:33:37AM +0800, Coly Li wrote:
On 2020/8/19 00:24, Christoph Hellwig wrote:
I think we should go for something simple like this instead:
This idea is fine to me. Should a warning message be through here? IMHO the driver still sends an improper page in, fix it in silence is too kind or over nice to the buggy driver(s).
I don't think a warning is a good idea. An API that does the right thing underneath and doesn't require boiler plate code in most callers is the right API.
On 2020/8/19 03:49, Christoph Hellwig wrote:
On Wed, Aug 19, 2020 at 12:33:37AM +0800, Coly Li wrote:
On 2020/8/19 00:24, Christoph Hellwig wrote:
I think we should go for something simple like this instead:
This idea is fine to me. Should a warning message be through here? IMHO the driver still sends an improper page in, fix it in silence is too kind or over nice to the buggy driver(s).
I don't think a warning is a good idea. An API that does the right thing underneath and doesn't require boiler plate code in most callers is the right API.
Then I don't have more comment.
Thanks.
Coly Li
On Wed, Aug 19, 2020 at 12:22:05PM +0800, Coly Li wrote:
On 2020/8/19 03:49, Christoph Hellwig wrote:
On Wed, Aug 19, 2020 at 12:33:37AM +0800, Coly Li wrote:
On 2020/8/19 00:24, Christoph Hellwig wrote:
I think we should go for something simple like this instead:
This idea is fine to me. Should a warning message be through here? IMHO the driver still sends an improper page in, fix it in silence is too kind or over nice to the buggy driver(s).
I don't think a warning is a good idea. An API that does the right thing underneath and doesn't require boiler plate code in most callers is the right API.
Then I don't have more comment.
So given the feedback from Dave I suspect we should actually resurrect this series, sorry for the noise. And in this case I think we do need the warning in kernel_sendpage.
On 2020/9/23 16:43, Christoph Hellwig wrote:
On Wed, Aug 19, 2020 at 12:22:05PM +0800, Coly Li wrote:
On 2020/8/19 03:49, Christoph Hellwig wrote:
On Wed, Aug 19, 2020 at 12:33:37AM +0800, Coly Li wrote:
On 2020/8/19 00:24, Christoph Hellwig wrote:
I think we should go for something simple like this instead:
This idea is fine to me. Should a warning message be through here? IMHO the driver still sends an improper page in, fix it in silence is too kind or over nice to the buggy driver(s).
I don't think a warning is a good idea. An API that does the right thing underneath and doesn't require boiler plate code in most callers is the right API.
Then I don't have more comment.
So given the feedback from Dave I suspect we should actually resurrect this series, sorry for the noise. And in this case I think we do need the warning in kernel_sendpage.
Copied, then I will post a v8 series, which adding a warning message in kernel_sendpage() if non-acceptible paage sent in.
Coly Li
linux-stable-mirror@lists.linaro.org
-
Christoph Hellwig -
Coly Li