[PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h

2020-08-18 Thread Coly Li
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 
Cc: Chaitanya Kulkarni 
Cc: Christoph Hellwig 
Cc: Hannes Reinecke 
Cc: Jan Kara 
Cc: Jens Axboe 
Cc: Mikhail Skorzhinskii 
Cc: Philipp Reisner 
Cc: Sagi Grimberg 
Cc: Vlastimil Babka 
Cc: sta...@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 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -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,
-- 
2.26.2

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20200818131227.37020-2-colyli%40suse.de.


Re: [PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h

2020-08-18 Thread Christoph Hellwig
I think we should go for something simple like this instead:

---
>From 4867e158ee86ebd801b4c267e8f8a4a762a71343 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig 
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 
---
 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);
-- 
2.28.0

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20200818162404.GA27196%40lst.de.


Re: [PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h

2020-08-18 Thread Christoph Hellwig
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.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20200818194930.GA31966%40lst.de.


Re: [PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h

2020-08-18 Thread Coly Li
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 
> 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 
> ---
>  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);
> 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/217ec0ec-3c5a-a8ed-27d9-c634f0b9a045%40suse.de.


Re: [PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h

2020-08-18 Thread Coly Li
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

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/04408ff6-f765-8f3e-ead9-aec55043e469%40suse.de.


Re: [PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h

2020-09-23 Thread Christoph Hellwig
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.

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/20200923084303.GA21657%40lst.de.


Re: [PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h

2020-09-23 Thread Coly Li
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

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/58455251-7d90-b890-17dc-abe0954715e1%40suse.de.