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
concrete zero-copy sendpage method in network layer.
The v10 series has 7 patches, fixes a WARN_ONCE() usage from v9 series,
- The 1st patch in this series introduces sendpage_ok() in header file
include/linux/net.h.
- The 2nd patch adds WARN_ONCE() for improper zero-copy send in
kernel_sendpage().
- The 3rd patch fixes the page checking issue in nvme-over-tcp driver.
- The 4th patch adds page_count check by using sendpage_ok() in
do_tcp_sendpages() as Eric Dumazet suggested.
- The 5th and 6th patches just replace existing open coded checks with
the inline sendpage_ok() routine.
Coly Li
Cc: Chaitanya Kulkarni <[email protected]>
Cc: Chris Leech <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Cong Wang <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Ilya Dryomov <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Jeff Layton <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Lee Duncan <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Mikhail Skorzhinskii <[email protected]>
Cc: Philipp Reisner <[email protected]>
Cc: Sagi Grimberg <[email protected]>
Cc: Vasily Averin <[email protected]>
Cc: Vlastimil Babka <[email protected]>
---
Changelog:
v10, fix WARN_ONCE() usage, and add Reivewed-by tag from Lee Duncan.
v9, fix a typo pointed out by Greg KH.
add Acked-by tags from Martin K. Petersen and Ilya Dryomov.
v8: add WARN_ONCE() in kernel_sendpage() as Christoph suggested.
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 (7):
net: introduce helper sendpage_ok() in include/linux/net.h
net: add WARN_ONCE in kernel_sendpage() for improper zero-copy send
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 ++-
net/socket.c | 6 ++++--
7 files changed, 28 insertions(+), 10 deletions(-)
--
2.26.2
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 <[email protected]>
Acked-by: Jeff Layton <[email protected]>
Cc: Ilya Dryomov <[email protected]>
---
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 bdfd66ba3843..d4d7a0e52491 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
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 <[email protected]>
Reviewed-by: Lee Duncan <[email protected]>
Acked-by: Martin K. Petersen <[email protected]>
Cc: Vasily Averin <[email protected]>
Cc: Cong Wang <[email protected]>
Cc: Mike Christie <[email protected]>
Cc: Chris Leech <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
---
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 37e5d4e48c2f..83f14b2c8804 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
From: Coly Li <[email protected]>
Date: Fri, 2 Oct 2020 16:27:27 +0800
> 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
> concrete zero-copy sendpage method in network layer.
Series applied.
> The v10 series has 7 patches, fixes a WARN_ONCE() usage from v9 series,
...
I still haven't heard from you how such a fundamental build failure
was even possible.
If the v9 patch series did not even compile, how in the world did you
perform functional testing of these changes?
Please explain this to me, instead of just quietly fixing it and
posting an updated series.
Thank you.
On 2020/10/3 06:28, David Miller wrote:
> From: Coly Li <[email protected]>
> Date: Fri, 2 Oct 2020 16:27:27 +0800
>
>> 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
>> concrete zero-copy sendpage method in network layer.
>
> Series applied.
>
>> The v10 series has 7 patches, fixes a WARN_ONCE() usage from v9 series,
> ...
>
> I still haven't heard from you how such a fundamental build failure
> was even possible.
>
Hi David,
Here is the detail steps how I leaked this uncompleted patch to you,
1) Add WARN_ONCE() as WARN_ON() to kernel_sendpage(). Maybe I was still
hesitating when I typed WARN_ONCE() on keyboard.
2) Generate the patches, prepare to post
3) Hmm, compiling failed, oh it is WARN_ONCE(). Yeah, WARN_ONCE() might
be more informative and better.
4) Modify to use WARN_ONCE() and compile and try, looks fine.
5) Re-generate the patches to overwrite the previous ones.
6) Post the patches.
The missing part was, before I post the patches, I should do rebase and
commit the change, but (interrupted by other stuffs) it skipped in my
mind. Although I regenerated the series but the change was not included.
The result was, uncompleted patch posted and the second-half change
still stayed in my local file.
> If the v9 patch series did not even compile, how in the world did you
> perform functional testing of these changes?
>
Only 0002-net-add-WARN_ONCE-in-kernel_sendpage-for-improper-ze.patch was
tested in v9 series, other tests were done in previous versions.
> Please explain this to me, instead of just quietly fixing it and
> posting an updated series.
And not all the patches in the series were tested. Here is the testing
coverage of the series:
The following ones were tested and verified to break nothing and avoid
the mm corruption and panic,
0001-net-introduce-helper-sendpage_ok-in-include-linux-ne.patch
0002-net-add-WARN_ONCE-in-kernel_sendpage-for-improper-ze.patch
0003-nvme-tcp-check-page-by-sendpage_ok-before-calling-ke.patch
0006-scsi-libiscsi-use-sendpage_ok-in-iscsi_tcp_segment_m.patch
The following ones were not tested, due to complicated environment setup,
0005-drbd-code-cleanup-by-using-sendpage_ok-to-check-page.patch
0007-libceph-use-sendpage_ok-in-ceph_tcp_sendpage.patch
This patch I didn't explicitly test, due to lack of knowledge to modify
network code to trigger a buggy condition. It just went with other
tested patches,
0004-tcp-use-sendpage_ok-to-detect-misused-.sendpage.patch
Back to the built failure, I don't have excuse for leaking this
uncompleted version to you. Of cause I will try to avoid to
inefficiently occupy maintainer's time by such silly mess up.
Thanks for your review and the thorough maintenance.
Coly Li
On 2020/10/3 06:28, David Miller wrote:
> From: Coly Li <[email protected]>
> Date: Fri, 2 Oct 2020 16:27:27 +0800
>
>> 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
>> concrete zero-copy sendpage method in network layer.
>
> Series applied.
>
>> The v10 series has 7 patches, fixes a WARN_ONCE() usage from v9 series,
> ...
>
> I still haven't heard from you how such a fundamental build failure
> was even possible.
>
Hi David,
Here is the detail steps how I leaked this uncompleted patch to you,
1) Add WARN_ONCE() as WARN_ON() to kernel_sendpage(). Maybe I was still
hesitating when I typed WARN_ONCE() on keyboard.
2) Generate the patches, prepare to post
3) Hmm, compiling failed, oh it is WARN_ONCE(). Yeah, WARN_ONCE() might
be more informative and better.
4) Modify to use WARN_ONCE() and compile and try, looks fine.
5) Re-generate the patches to overwrite the previous ones.
6) Post the patches.
The missing part was, before I post the patches, I should do rebase and
commit the change, but (interrupted by other stuffs) it skipped in my
mind. Although I regenerated the series but the change was not included.
The result was, uncompleted patch posted and the second-half change
still stayed in my local file.
> If the v9 patch series did not even compile, how in the world did you
> perform functional testing of these changes?
>
Only 0002-net-add-WARN_ONCE-in-kernel_sendpage-for-improper-ze.patch was
tested in v9 series, other tests were done in previous versions.
> Please explain this to me, instead of just quietly fixing it and
> posting an updated series.
And not all the patches in the series were tested. Here is the testing
coverage of the series:
The following ones were tested and verified to break nothing and avoid
the mm corruption and panic,
0001-net-introduce-helper-sendpage_ok-in-include-linux-ne.patch
0002-net-add-WARN_ONCE-in-kernel_sendpage-for-improper-ze.patch
0003-nvme-tcp-check-page-by-sendpage_ok-before-calling-ke.patch
0006-scsi-libiscsi-use-sendpage_ok-in-iscsi_tcp_segment_m.patch
The following ones were not tested, due to complicated environment setup,
0005-drbd-code-cleanup-by-using-sendpage_ok-to-check-page.patch
0007-libceph-use-sendpage_ok-in-ceph_tcp_sendpage.patch
This patch I didn't explicitly test, due to lack of knowledge to modify
network code to trigger a buggy condition. It just went with other
tested patches,
0004-tcp-use-sendpage_ok-to-detect-misused-.sendpage.patch
Back to the built failure, I don't have excuse for leaking this
uncompleted version to you. Of cause I will try to avoid to
inefficiently occupy maintainer's time by such silly mess up.
Thanks for your review and the thorough maintenance.
Coly Li