[PATCH v7 0/6] Introduce sendpage_ok() to detect misused sendpage in network related drivers

[Coly Li]

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 <chaitanya...@wdc.com>
Cc: Chris Leech <cle...@redhat.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: Cong Wang <amw...@redhat.com>
Cc: David S. Miller <da...@davemloft.net>
Cc: Eric Dumazet <eric.d...@gmail.com>
Cc: Hannes Reinecke <ha...@suse.de>
Cc: Ilya Dryomov <idry...@gmail.com>
Cc: Jan Kara <ja...@suse.com>
Cc: Jeff Layton <jla...@kernel.org>
Cc: Jens Axboe <ax...@kernel.dk>
Cc: Lee Duncan <ldu...@suse.com>
Cc: Mike Christie <mich...@cs.wisc.edu>
Cc: Mikhail Skorzhinskii <mskorz...@solarflare.com>
Cc: Philipp Reisner <philipp...@linbit.com>
Cc: Sagi Grimberg <sa...@grimberg.me>
Cc: Vasily Averin <v...@virtuozzo.com>
Cc: Vlastimil Babka <vba...@suse.com>
---
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

[Coly Li]

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 <col...@suse.de>
Cc: Chaitanya Kulkarni <chaitanya...@wdc.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: Hannes Reinecke <ha...@suse.de>
Cc: Jan Kara <ja...@suse.com>
Cc: Jens Axboe <ax...@kernel.dk>

Cc: Mikhail Skorzhinskii <mskorz...@solarflare.com>
Cc: Philipp Reisner <philipp...@linbit.com>
Cc: Sagi Grimberg <sa...@grimberg.me>

Cc: Vlastimil Babka <vba...@suse.com>
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

[Coly Li]

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 <eric.d...@gmail.com>
Signed-off-by: Coly Li <col...@suse.de>
Cc: Vasily Averin <v...@virtuozzo.com>

Cc: David S. Miller <da...@davemloft.net>

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

[Coly Li]

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 <chaitanya...@wdc.com>
Cc: Chris Leech <cle...@redhat.com>
Cc: Christoph Hellwig <h...@lst.de>
Cc: Cong Wang <amw...@redhat.com>

Cc: David S. Miller <da...@davemloft.net>

Cc: Hannes Reinecke <ha...@suse.de>
Cc: Ilya Dryomov <idry...@gmail.com>

Cc: Jan Kara <ja...@suse.com>
Cc: Jeff Layton <jla...@kernel.org>
Cc: Jens Axboe <ax...@kernel.dk>

Cc: Lee Duncan <ldu...@suse.com>
Cc: Mike Christie <mich...@cs.wisc.edu>

Cc: Mikhail Skorzhinskii <mskorz...@solarflare.com>
Cc: Philipp Reisner <philipp...@linbit.com>
Cc: Sagi Grimberg <sa...@grimberg.me>

Cc: sta...@vger.kernel.org
Cc: Vasily Averin <v...@virtuozzo.com>

Cc: Vlastimil Babka <vba...@suse.com>
---
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()

[Coly Li]

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 <col...@suse.de>

Cc: Philipp Reisner <philipp...@linbit.com>
Cc: Sagi Grimberg <sa...@grimberg.me>

---
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