Re: Concurrent logins to different interfaces of same iscsi target and login timeout
Hi Lee, Thanks for adding the async login support to upstream. I've ran some tests using the iscsiadm built from there and would like to ask: 1. How is it possible to gather the async logins return status? if understood correctly, the proposed way is to lookup for the connections in the output of "iscsiadm -m session" after the async logins were launched. Currently, I am using a sampling loop, checking at 1 second intervals the output of iscsiadm -m session for presence of expected connections targets and portals and breaks if all were found or not found within the expected timeout interval, which for the default iscsi settings is considered as following: (120 seconds timeout per connection login) * (number of connections) / (number of workers) Is there a better way? I am not sure how to gather the error status when a connection not able to login in such case. 2. Would it also be supported for non-login-all mode? For "iscsiadm -m node -T target -p portal -I interface --login" I get same timeouts with/without the --no-wait flag, meaning the test waits 240 seconds in case two connections are down when using a single node login worker for both cases, so I assume it currently doesn't apply for this login mode. -- Simulating one portal down (2 connections down) with one worker, using node login without --no-wait # python3 ./initiator.py -j 1 -i 10.35.18.220 10.35.18.156 -d 10.35.18.156 2020-08-18 15:59:01,874 INFO(MainThread) Removing prior sessions and nodes 2020-08-18 15:59:01,882 INFO(MainThread) Deleting all nodes 2020-08-18 15:59:01,893 INFO(MainThread) No active sessions 2020-08-18 15:59:01,943 INFO(MainThread) Setting 10.35.18.156 as invalid address for target iqn.2003-01.org.vm-18-220.iqn2 2020-08-18 15:59:01,943 INFO(MainThread) Setting 10.35.18.156 as invalid address for target iqn.2003-01.org.vm-18-220.iqn1 2020-08-18 15:59:01,943 INFO(MainThread) Discovered connections: {('iqn.2003-01.org.vm-18-220.iqn1', '0.0.0.0:0,0'), ('iqn.2003-01.org.vm-18-220.iqn2', '0.0.0.0:0,0'), ('iqn.2003-01.org.vm-18-220.iqn2', '10.35.18.220:3260,1'), ('iqn.2003-01.org.vm-18-220.iqn1', '10.35.18.220:3260,1')} 2020-08-18 15:59:01,944 INFO(MainThread) Adding node for target iqn.2003-01.org.vm-18-220.iqn1 portal 0.0.0.0:0,0 2020-08-18 15:59:01,956 INFO(MainThread) Adding node for target iqn.2003-01.org.vm-18-220.iqn2 portal 0.0.0.0:0,0 2020-08-18 15:59:01,968 INFO(MainThread) Adding node for target iqn.2003-01.org.vm-18-220.iqn2 portal 10.35.18.220:3260,1 2020-08-18 15:59:01,980 INFO(MainThread) Adding node for target iqn.2003-01.org.vm-18-220.iqn1 portal 10.35.18.220:3260,1 2020-08-18 15:59:01,995 INFO(login_0) Login to target iqn.2003-01.org.vm-18-220.iqn1 portal 0.0.0.0:0,0 (nowait=False) 2020-08-18 16:01:02,019 INFO(login_0) Login to target iqn.2003-01.org.vm-18-220.iqn2 portal 0.0.0.0:0,0 (nowait=False) 2020-08-18 16:01:02,028 ERROR (MainThread) Job failed: Command ['iscsiadm', '--mode', 'node', '--targetname', 'iqn.2003-01.org.vm-18-220.iqn1', '--interface', 'default', '--portal', '0.0.0.0:0,0', '--login'] failed rc=8 out='Logging in to [iface: default, target: iqn.2003-01.org.vm-18-220.iqn1, portal: 0.0.0.0,0]' err='iscsiadm: Could not login to [iface: default, target: iqn.2003-01.org.vm-18-220.iqn1, portal: 0.0.0.0,0].\niscsiadm: initiator reported error (8 - connection timed out)\niscsiadm: Could not log into all portals' 2020-08-18 16:03:02,045 INFO(login_0) Login to target iqn.2003-01.org.vm-18-220.iqn2 portal 10.35.18.220:3260,1 (nowait=False) 2020-08-18 16:03:02,053 ERROR (MainThread) Job failed: Command ['iscsiadm', '--mode', 'node', '--targetname', 'iqn.2003-01.org.vm-18-220.iqn2', '--interface', 'default', '--portal', '0.0.0.0:0,0', '--login'] failed rc=8 out='Logging in to [iface: default, target: iqn.2003-01.org.vm-18-220.iqn2, portal: 0.0.0.0,0]' err='iscsiadm: Could not login to [iface: default, target: iqn.2003-01.org.vm-18-220.iqn2, portal: 0.0.0.0,0].\niscsiadm: initiator reported error (8 - connection timed out)\niscsiadm: Could not log into all portals' 2020-08-18 16:03:02,321 INFO(login_0) Login to target iqn.2003-01.org.vm-18-220.iqn1 portal 10.35.18.220:3260,1 (nowait=False) 2020-08-18 16:03:02,695 INFO(MainThread) Connecting completed in 240.752s -- Simulating one portal down (2 connections down) with one worker, using node login with --no-wait # python3 ./initiator.py -j 1 -i 10.35.18.220 10.35.18.156 -d 10.35.18.156 --nowait 2020-08-18 16:16:05,802 INFO(MainThread) Removing prior sessions and nodes 2020-08-18 16:16:06,075 INFO(MainThread) Deleting all nodes 2020-08-18 16:16:06,090 INFO(MainThread) No active sessions 2020-08-18 16:16:06,130 INFO(MainThread) Setting 10.35.18.156 as invalid address for target iqn.2003-01.org.vm-18-220.iqn2 2020-08-18 16:16:06,131 INFO(MainThread) Setting 10.35.18.156 as invalid address for target iqn.2003-01.org.vm-18
Re: [PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h
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
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.
[PATCH v6 1/6] net: introduce helper sendpage_ok() in include/linux/net.h
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..a807fad31958 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/20200818124736.5790-2-colyli%40suse.de.
[PATCH v7 6/6] libceph: use sendpage_ok() in ceph_tcp_sendpage()
In libceph, ceph_tcp_sendpage() does the following checks before handle the page by network layer's zero copy sendpage method, if (page_count(page) >= 1 && !PageSlab(page)) This check is exactly what sendpage_ok() does. This patch replace the open coded checks by sendpage_ok() as a code cleanup. Signed-off-by: Coly Li Cc: Ilya Dryomov Cc: Jeff Layton --- net/ceph/messenger.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c index 27d6ab11f9ee..6a349da7f013 100644 --- a/net/ceph/messenger.c +++ b/net/ceph/messenger.c @@ -575,7 +575,7 @@ static int ceph_tcp_sendpage(struct socket *sock, struct page *page, * coalescing neighboring slab objects into a single frag which * triggers one of hardened usercopy checks. */ - if (page_count(page) >= 1 && !PageSlab(page)) + if (sendpage_ok(page)) sendpage = sock->ops->sendpage; else sendpage = sock_no_sendpage; -- 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-7-colyli%40suse.de.
[PATCH v7 2/6] nvme-tcp: check page by sendpage_ok() before calling kernel_sendpage()
Currently nvme_tcp_try_send_data() doesn't use kernel_sendpage() to send slab pages. But for pages allocated by __get_free_pages() without __GFP_COMP, which also have refcount as 0, they are still sent by kernel_sendpage() to remote end, this is problematic. The new introduced helper sendpage_ok() checks both PageSlab tag and page_count counter, and returns true if the checking page is OK to be sent by kernel_sendpage(). This patch fixes the page checking issue of nvme_tcp_try_send_data() with sendpage_ok(). If sendpage_ok() returns true, send this page by kernel_sendpage(), otherwise use sock_no_sendpage to handle this 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 --- drivers/nvme/host/tcp.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index 62fbaecdc960..902fe742762b 100644 --- a/drivers/nvme/host/tcp.c +++ b/drivers/nvme/host/tcp.c @@ -912,12 +912,11 @@ static int nvme_tcp_try_send_data(struct nvme_tcp_request *req) else flags |= MSG_MORE | MSG_SENDPAGE_NOTLAST; - /* can't zcopy slab pages */ - if (unlikely(PageSlab(page))) { - ret = sock_no_sendpage(queue->sock, page, offset, len, + if (sendpage_ok(page)) { + ret = kernel_sendpage(queue->sock, page, offset, len, flags); } else { - ret = kernel_sendpage(queue->sock, page, offset, len, + ret = sock_no_sendpage(queue->sock, page, offset, len, flags); } if (ret <= 0) -- 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-3-colyli%40suse.de.
[PATCH v6 0/6] Introduce sendpage_ok() to detect misused sendpage in network related drivers
This series was original by a bug fix in nvme-over-tcp driver which only checked whether a page was allocated from slab allcoator, but forgot to check its page_count: The page handled by sendpage should be neither a Slab page nor 0 page_count page. As Sagi Grimberg suggested, the original fix is refind to a more common inline routine: static inline bool sendpage_ok(struct page *page) { return (!PageSlab(page) && page_count(page) >= 1); } If sendpage_ok() returns true, the checking page can be handled by the zero copy sendpage method in network layer. The first patch in this series introduces sendpage_ok() in header file include/linux/net.h, the second patch fixes the page checking issue in nvme-over-tcp driver, the third patch adds page_count check by using sendpage_ok() in do_tcp_sendpages() as Eric Dumazet suggested, and all rested patches just replace existing open coded checks with the inline sendpage_ok() routine. Coly Li Cc: Chaitanya Kulkarni Cc: Chris Leech Cc: Christoph Hellwig Cc: Cong Wang Cc: David S. Miller Cc: Hannes Reinecke Cc: Ilya Dryomov Cc: Jan Kara Cc: Jeff Layton Cc: Jens Axboe Cc: Lee Duncan Cc: Mike Christie Cc: Mikhail Skorzhinskii Cc: Philipp Reisner Cc: Sagi Grimberg Cc: sta...@vger.kernel.org Cc: Vasily Averin Cc: Vlastimil Babka --- Changelog: v6: fix page check in do_tcp_sendpages(), and replace other open coded checks with sendpage_ok() in libceph, iscsi drivers. v5, include linux/mm.h in include/linux/net.h v4, change sendpage_ok() as an inline helper, and post it as separate patch. v3, introduce a more common sendpage_ok() v2, fix typo in patch subject v1, the initial version. Coly Li (6): net: introduce helper sendpage_ok() in include/linux/net.h nvme-tcp: check page by sendpage_ok() before calling kernel_sendpage() tcp: use sendpage_ok() to detect misused .sendpage drbd: code cleanup by using sendpage_ok() to check page for kernel_sendpage() scsi: libiscsi: use sendpage_ok() in iscsi_tcp_segment_map() libceph: use sendpage_ok() in ceph_tcp_sendpage() drivers/block/drbd/drbd_main.c | 2 +- drivers/nvme/host/tcp.c| 7 +++ drivers/scsi/libiscsi_tcp.c| 2 +- include/linux/net.h| 16 net/ceph/messenger.c | 2 +- net/ipv4/tcp.c | 3 ++- 6 files changed, 24 insertions(+), 8 deletions(-) -- 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/20200818124736.5790-1-colyli%40suse.de.
[PATCH v6 5/6] scsi: libiscsi: use sendpage_ok() in iscsi_tcp_segment_map()
In iscsci driver, iscsi_tcp_segment_map() uses the following code to check whether the page should or not be handled by sendpage: if (!recv && page_count(sg_page(sg)) >= 1 && !PageSlab(sg_page(sg))) The "page_count(sg_page(sg)) >= 1 && !PageSlab(sg_page(sg)" part is to make sure the page can be sent to network layer's zero copy path. This part is exactly what sendpage_ok() does. This patch uses use sendpage_ok() in iscsi_tcp_segment_map() to replace the original open coded checks. Signed-off-by: Coly Li Cc: Vasily Averin Cc: Cong Wang Cc: Mike Christie Cc: Lee Duncan Cc: Chris Leech Cc: Christoph Hellwig Cc: Hannes Reinecke --- drivers/scsi/libiscsi_tcp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c index 6ef93c7af954..31cd8487c16e 100644 --- a/drivers/scsi/libiscsi_tcp.c +++ b/drivers/scsi/libiscsi_tcp.c @@ -128,7 +128,7 @@ static void iscsi_tcp_segment_map(struct iscsi_segment *segment, int recv) * coalescing neighboring slab objects into a single frag which * triggers one of hardened usercopy checks. */ - if (!recv && page_count(sg_page(sg)) >= 1 && !PageSlab(sg_page(sg))) + if (!recv && sendpage_ok(sg_page(sg))) return; if (recv) { -- 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/20200818124736.5790-6-colyli%40suse.de.
Re: [PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h
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.
[PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h
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.
[PATCH v6 4/6] drbd: code cleanup by using sendpage_ok() to check page for kernel_sendpage()
In _drbd_send_page() a page is checked by following code before sending it by kernel_sendpage(), (page_count(page) < 1) || PageSlab(page) If the check is true, this page won't be send by kernel_sendpage() and handled by sock_no_sendpage(). This kind of check is exactly what macro sendpage_ok() does, which is introduced into include/linux/net.h to solve a similar send page issue in nvme-tcp code. This patch uses macro sendpage_ok() to replace the open coded checks to page type and refcount in _drbd_send_page(), as a code cleanup. Signed-off-by: Coly Li Cc: Philipp Reisner Cc: Sagi Grimberg --- drivers/block/drbd/drbd_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c index cb687ccdbd96..55dc0c91781e 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -1553,7 +1553,7 @@ static int _drbd_send_page(struct drbd_peer_device *peer_device, struct page *pa * 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. */ - if (drbd_disable_sendpage || (page_count(page) < 1) || PageSlab(page)) + if (drbd_disable_sendpage || !sendpage_ok(page)) return _drbd_no_send_page(peer_device, page, offset, size, msg_flags); msg_flags |= MSG_NOSIGNAL; -- 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/20200818124736.5790-5-colyli%40suse.de.
[PATCH v6 2/6] nvme-tcp: check page by sendpage_ok() before calling kernel_sendpage()
Currently nvme_tcp_try_send_data() doesn't use kernel_sendpage() to send slab pages. But for pages allocated by __get_free_pages() without __GFP_COMP, which also have refcount as 0, they are still sent by kernel_sendpage() to remote end, this is problematic. The new introduced helper sendpage_ok() checks both PageSlab tag and page_count counter, and returns true if the checking page is OK to be sent by kernel_sendpage(). This patch fixes the page checking issue of nvme_tcp_try_send_data() with sendpage_ok(). If sendpage_ok() returns true, send this page by kernel_sendpage(), otherwise use sock_no_sendpage to handle this 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 --- drivers/nvme/host/tcp.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index 62fbaecdc960..902fe742762b 100644 --- a/drivers/nvme/host/tcp.c +++ b/drivers/nvme/host/tcp.c @@ -912,12 +912,11 @@ static int nvme_tcp_try_send_data(struct nvme_tcp_request *req) else flags |= MSG_MORE | MSG_SENDPAGE_NOTLAST; - /* can't zcopy slab pages */ - if (unlikely(PageSlab(page))) { - ret = sock_no_sendpage(queue->sock, page, offset, len, + if (sendpage_ok(page)) { + ret = kernel_sendpage(queue->sock, page, offset, len, flags); } else { - ret = kernel_sendpage(queue->sock, page, offset, len, + ret = sock_no_sendpage(queue->sock, page, offset, len, flags); } if (ret <= 0) -- 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/20200818124736.5790-3-colyli%40suse.de.
[PATCH v7 4/6] drbd: code cleanup by using sendpage_ok() to check page for kernel_sendpage()
In _drbd_send_page() a page is checked by following code before sending it by kernel_sendpage(), (page_count(page) < 1) || PageSlab(page) If the check is true, this page won't be send by kernel_sendpage() and handled by sock_no_sendpage(). This kind of check is exactly what macro sendpage_ok() does, which is introduced into include/linux/net.h to solve a similar send page issue in nvme-tcp code. This patch uses macro sendpage_ok() to replace the open coded checks to page type and refcount in _drbd_send_page(), as a code cleanup. Signed-off-by: Coly Li Cc: Philipp Reisner Cc: Sagi Grimberg --- drivers/block/drbd/drbd_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c index cb687ccdbd96..55dc0c91781e 100644 --- a/drivers/block/drbd/drbd_main.c +++ b/drivers/block/drbd/drbd_main.c @@ -1553,7 +1553,7 @@ static int _drbd_send_page(struct drbd_peer_device *peer_device, struct page *pa * 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. */ - if (drbd_disable_sendpage || (page_count(page) < 1) || PageSlab(page)) + if (drbd_disable_sendpage || !sendpage_ok(page)) return _drbd_no_send_page(peer_device, page, offset, size, msg_flags); msg_flags |= MSG_NOSIGNAL; -- 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-5-colyli%40suse.de.
[PATCH v7 3/6] tcp: use sendpage_ok() to detect misused .sendpage
commit a10674bf2406 ("tcp: detecting the misuse of .sendpage for Slab objects") adds the checks for Slab pages, but the pages don't have page_count are still missing from the check. Network layer's sendpage method is not designed to send page_count 0 pages neither, therefore both PageSlab() and page_count() should be both checked for the sending page. This is exactly what sendpage_ok() does. This patch uses sendpage_ok() in do_tcp_sendpages() to detect misused .sendpage, to make the code more robust. Fixes: a10674bf2406 ("tcp: detecting the misuse of .sendpage for Slab objects") Suggested-by: Eric Dumazet Signed-off-by: Coly Li Cc: Vasily Averin Cc: David S. Miller Cc: sta...@vger.kernel.org --- net/ipv4/tcp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 31f3b858db81..2135ee7c806d 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -970,7 +970,8 @@ ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset, long timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT); if (IS_ENABLED(CONFIG_DEBUG_VM) && - WARN_ONCE(PageSlab(page), "page must not be a Slab one")) + WARN_ONCE(!sendpage_ok(page), + "page must not be a Slab one and have page_count > 0")) return -EINVAL; /* Wait for a connection to finish. One exception is TCP Fast Open -- 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-4-colyli%40suse.de.
[PATCH v6 3/6] tcp: use sendpage_ok() to detect misused .sendpage
commit a10674bf2406 ("tcp: detecting the misuse of .sendpage for Slab objects") adds the checks for Slab pages, but the pages don't have page_count are still missing from the check. Network layer's sendpage method is not designed to send page_count 0 pages neither, therefore both PageSlab() and page_count() should be both checked for the sending page. This is exactly what sendpage_ok() does. This patch uses sendpage_ok() in do_tcp_sendpages() to detect misused .sendpage, to make the code more robust. Fixes: a10674bf2406 ("tcp: detecting the misuse of .sendpage for Slab objects") Suggested-by: Eric Dumazet Signed-off-by: Coly Li Cc: Vasily Averin Cc: David S. Miller Cc: sta...@vger.kernel.org --- net/ipv4/tcp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 31f3b858db81..d96c7549895a 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -970,7 +970,8 @@ ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset, long timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT); if (IS_ENABLED(CONFIG_DEBUG_VM) && - WARN_ONCE(PageSlab(page), "page must not be a Slab one")) + WARN_ONCE(!sendpage_ok(page)), + "page must not be a Slab one and have page_count > 0") return -EINVAL; /* Wait for a connection to finish. One exception is TCP Fast Open -- 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/20200818124736.5790-4-colyli%40suse.de.
[PATCH v6 6/6] libceph: use sendpage_ok() in ceph_tcp_sendpage()
In libceph, ceph_tcp_sendpage() does the following checks before handle the page by network layer's zero copy sendpage method, if (page_count(page) >= 1 && !PageSlab(page)) This check is exactly what sendpage_ok() does. This patch replace the open coded checks by sendpage_ok() as a code cleanup. Signed-off-by: Coly Li Cc: Ilya Dryomov Cc: Jeff Layton --- net/ceph/messenger.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c index 27d6ab11f9ee..6a349da7f013 100644 --- a/net/ceph/messenger.c +++ b/net/ceph/messenger.c @@ -575,7 +575,7 @@ static int ceph_tcp_sendpage(struct socket *sock, struct page *page, * coalescing neighboring slab objects into a single frag which * triggers one of hardened usercopy checks. */ - if (page_count(page) >= 1 && !PageSlab(page)) + if (sendpage_ok(page)) sendpage = sock->ops->sendpage; else sendpage = sock_no_sendpage; -- 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/20200818124736.5790-7-colyli%40suse.de.
[PATCH v7 0/6] Introduce sendpage_ok() to detect misused sendpage in network related drivers
This series was original by a bug fix in nvme-over-tcp driver which only checked whether a page was allocated from slab allcoator, but forgot to check its page_count: The page handled by sendpage should be neither a Slab page nor 0 page_count page. As Sagi Grimberg suggested, the original fix is refind to a more common inline routine: static inline bool sendpage_ok(struct page *page) { return (!PageSlab(page) && page_count(page) >= 1); } If sendpage_ok() returns true, the checking page can be handled by the zero copy sendpage method in network layer. The first patch in this series introduces sendpage_ok() in header file include/linux/net.h, the second patch fixes the page checking issue in nvme-over-tcp driver, the third patch adds page_count check by using sendpage_ok() in do_tcp_sendpages() as Eric Dumazet suggested, and all rested patches just replace existing open coded checks with the inline sendpage_ok() routine. Coly Li Cc: Chaitanya Kulkarni Cc: Chris Leech Cc: Christoph Hellwig Cc: Cong Wang Cc: David S. Miller Cc: Eric Dumazet Cc: Hannes Reinecke Cc: Ilya Dryomov Cc: Jan Kara Cc: Jeff Layton Cc: Jens Axboe Cc: Lee Duncan Cc: Mike Christie Cc: Mikhail Skorzhinskii Cc: Philipp Reisner Cc: Sagi Grimberg Cc: Vasily Averin Cc: Vlastimil Babka --- Changelog: v7: remove outer brackets from the return line of sendpage_ok() as Eric Dumazet suggested. v6: fix page check in do_tcp_sendpages(), as Eric Dumazet suggested. replace other open coded checks with sendpage_ok() in libceph, iscsi drivers. v5, include linux/mm.h in include/linux/net.h v4, change sendpage_ok() as an inline helper, and post it as separate patch, as Christoph Hellwig suggested. v3, introduce a more common sendpage_ok() as Sagi Grimberg suggested. v2, fix typo in patch subject v1, the initial version. Coly Li (6): net: introduce helper sendpage_ok() in include/linux/net.h nvme-tcp: check page by sendpage_ok() before calling kernel_sendpage() tcp: use sendpage_ok() to detect misused .sendpage drbd: code cleanup by using sendpage_ok() to check page for kernel_sendpage() scsi: libiscsi: use sendpage_ok() in iscsi_tcp_segment_map() libceph: use sendpage_ok() in ceph_tcp_sendpage() drivers/block/drbd/drbd_main.c | 2 +- drivers/nvme/host/tcp.c| 7 +++ drivers/scsi/libiscsi_tcp.c| 2 +- include/linux/net.h| 16 net/ceph/messenger.c | 2 +- net/ipv4/tcp.c | 3 ++- 6 files changed, 24 insertions(+), 8 deletions(-) -- 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-1-colyli%40suse.de.
[PATCH v7 5/6] scsi: libiscsi: use sendpage_ok() in iscsi_tcp_segment_map()
In iscsci driver, iscsi_tcp_segment_map() uses the following code to check whether the page should or not be handled by sendpage: if (!recv && page_count(sg_page(sg)) >= 1 && !PageSlab(sg_page(sg))) The "page_count(sg_page(sg)) >= 1 && !PageSlab(sg_page(sg)" part is to make sure the page can be sent to network layer's zero copy path. This part is exactly what sendpage_ok() does. This patch uses use sendpage_ok() in iscsi_tcp_segment_map() to replace the original open coded checks. Signed-off-by: Coly Li Cc: Vasily Averin Cc: Cong Wang Cc: Mike Christie Cc: Lee Duncan Cc: Chris Leech Cc: Christoph Hellwig Cc: Hannes Reinecke --- drivers/scsi/libiscsi_tcp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c index 6ef93c7af954..31cd8487c16e 100644 --- a/drivers/scsi/libiscsi_tcp.c +++ b/drivers/scsi/libiscsi_tcp.c @@ -128,7 +128,7 @@ static void iscsi_tcp_segment_map(struct iscsi_segment *segment, int recv) * coalescing neighboring slab objects into a single frag which * triggers one of hardened usercopy checks. */ - if (!recv && page_count(sg_page(sg)) >= 1 && !PageSlab(sg_page(sg))) + if (!recv && sendpage_ok(sg_page(sg))) return; if (recv) { -- 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-6-colyli%40suse.de.
Re: [PATCH v7 1/6] net: introduce helper sendpage_ok() in include/linux/net.h
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.
Antw: [EXT] [PATCH v6 1/6] net: introduce helper sendpage_ok() in include/linux/net.h
>>> Coly Li schrieb am 18.08.2020 um 14:47 in Nachricht <20200818124736.5790-2-col...@suse.de>: > 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..a807fad31958 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. > + */ Actually I think this comment is somewhat mis-placed: It should describe what the function does (check for specific properties of a page), but not where this function might be used. Most notably, because the use (from where it is called) may change over time, while the function will still do the same thing. > +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/20200818124736.5790-2-colyli%40s > use.de. -- 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/5F3CBF5302A10003AB07%40gwsmtp.uni-regensburg.de.