Hello, How about use WARN_ONCE() just in case to catch any unexpected changes in the future? Though page array is not used in bvec now, we can't guarantee it won't change in the future.
Wrt whether it is necessary to check page ref = 0 here, I have same doubts, I also don't quite understand why it is possible that 0-ref page would end up here, but if that is possible, it should be handled correctly with this patch. BTW, do you think we need to replace iov_iter_get_pages() in do_recv_one_seg() with similar implementation? -----Original Message----- From: Alexey Kuznetsov <kuz...@acronis.com <mailto:kuz...@acronis.com>> Date: Wednesday, 19 April 2023 at 12:09 AM To: Kui Liu <kui....@acronis.com <mailto:kui....@acronis.com>> Cc: Devel <devel@openvz.org <mailto:devel@openvz.org>>, Konstantin Khorenko <khore...@virtuozzo.com <mailto:khore...@virtuozzo.com>> Subject: Re: [Devel] [PATCH RH9] fuse: pcs: do not use sendpage on pages which cannot be sendpaged Hello! Ack. Disgusting at the first sight, but I like this! :-) Indeed, this iterator is our internal thing, we created it ourselves and actually we have no reasons to use dubious spoiled system implementation. One note, formally, BUG_ON is incorrect. We should assert it->iov_offset + i->bvec->bv_offset < PAGE_SIZE. I do not understand this place in system implementation, I have never seen anyone using bvecs in this way, they allow bv_page pointing to array of pages, this looks so scary. But this is our internal iter, so that we may omit assertions. BTW this does not answer my doubts, the problem is not in our code. I do not understand _why_ sendpage_ok is necessary. The check page_count(page) >= 1 wants to tell that get_page on page with 0 refcnt is illegal. But fuse already did get_page at top level, it uses iov_iter_get_pages in fuse_get_user_pages(), so that: 1. If it is not allowed it is already broken by the time the page arrives to us 2. It is impossible to have page_count == 0 in kernel_sendpage, it is grabbed by top level. All this looks as big smelly pile of shit, mainstream not us. :-) On Tue, Apr 18, 2023 at 10:22 PM Kui Liu <kui....@acronis.com <mailto:kui....@acronis.com>> wrote: > > > It looks like the new API iov_iter_get_pages is not safe for use > when trying to get a page from a bvec to be sent by kernel_sendpage(). > So just revert back our own implentation where we check the page > before making get_page(), if the page can't be sendpage, fall back > to copy mode. > > Affects: #PSBM-146821, #PSBM-146846 > https://jira.vzint.dev/browse/PSBM-146821 > <https://jira.vzint.dev/browse/PSBM-146821> > https://jira.vzint.dev/browse/PSBM-146846 > <https://jira.vzint.dev/browse/PSBM-146846> > Signed-off-by: Liu Kui <kui....@acronis.com <mailto:kui....@acronis.com>> > --- > fs/fuse/kio/pcs/pcs_sock_io.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/fs/fuse/kio/pcs/pcs_sock_io.c b/fs/fuse/kio/pcs/pcs_sock_io.c > index 552b82ab398b..20f088d36d6d 100644 > --- a/fs/fuse/kio/pcs/pcs_sock_io.c > +++ b/fs/fuse/kio/pcs/pcs_sock_io.c > @@ -143,16 +143,32 @@ static int do_send_one_seg(struct socket *sock, struct > iov_iter *it, size_t left > } > > if (iov_iter_is_bvec(it)) { > - /* Zerocopy */ > size_t offset; > ssize_t len; > struct page *page; > > - len = iov_iter_get_pages(it, &page, size, 1, &offset); > - BUG_ON(len <= 0); > - > - ret = kernel_sendpage(sock, page, offset, len, flags); > - put_page(page); > + /* Only support single page bvec here */ > + BUG_ON(it->bvec->bv_len > PAGE_SIZE); > + > + page = it->bvec->bv_page; > + offset = it->bvec->bv_offset + it->iov_offset; > + len = min(size, it->bvec->bv_len - it->iov_offset); > + > + if (sendpage_ok(page)) { > + /* Zero copy */ > + get_page(page); > + ret = kernel_sendpage(sock, page, offset, len, flags); > + put_page(page); > + } else { > + /* Fall back to copy mode */ > + struct msghdr msg = { .msg_flags = flags }; > + struct kvec kv; > + > + kv.iov_base = kmap(page) + offset; > + kv.iov_len = len; > + ret = kernel_sendmsg(sock, &msg, &kv, 1, size); > + kunmap(page); > + } > } > > out: > -- > 2.32.0 (Apple Git-132) > > -----Original Message----- > From: Alexey Kuznetsov <kuz...@acronis.com <mailto:kuz...@acronis.com> > <mailto:kuz...@acronis.com <mailto:kuz...@acronis.com>>> > Date: Monday, 17 April 2023 at 10:31 PM > To: Devel <devel@openvz.org <mailto:devel@openvz.org> > <mailto:devel@openvz.org <mailto:devel@openvz.org>>>, Konstantin Khorenko > <khore...@virtuozzo.com <mailto:khore...@virtuozzo.com> > <mailto:khore...@virtuozzo.com <mailto:khore...@virtuozzo.com>>>, Kui Liu > <kui....@acronis.com <mailto:kui....@acronis.com> <mailto:kui....@acronis.com > <mailto:kui....@acronis.com>>> > Subject: [PATCH RH9] fuse: pcs: do not use sendpage on pages which cannot be > sendpaged > > > Use the same approach as in iscsi iscsi_tcp_segment_map() which makes > the same operation > in the same context. > > > In vz7 we added iov_iter_get_pages() infrastructure ourselves and we > did it in safe way. > We checked page before doing any manipulations on it right in core of > iov_iter_get_pages() > and terminated iov_iter_get_pages() if the page looked dubious. We > cannot make this now because > this function is used in many places over kernel and not all the > places are sensitive > to origin of pages but all of them definitely will crash if > iov_iter_get_pages() returns error. > > > This patch will work for now. I still believe this place deserves > further investigation. > It smells like mainstream could be wrong. Look at my comment in code > for the reasons of doubts. > > > Affects: #PSBM-146821, #PSBM-146846 > https://jira.vzint.dev/browse/PSBM-146821 > <https://jira.vzint.dev/browse/PSBM-146821> > <https://jira.vzint.dev/browse/PSBM-146821> > <https://jira.vzint.dev/browse/PSBM-146821>> > https://jira.vzint.dev/browse/PSBM-146846 > <https://jira.vzint.dev/browse/PSBM-146846> > <https://jira.vzint.dev/browse/PSBM-146846> > <https://jira.vzint.dev/browse/PSBM-146846>> > > > Signed-off-by: Alexey Kuznetsov <kuz...@acronis.com > <mailto:kuz...@acronis.com> <mailto:kuz...@acronis.com > <mailto:kuz...@acronis.com>>> > > > > _______________________________________________ > Devel mailing list > Devel@openvz.org <mailto:Devel@openvz.org> > https://lists.openvz.org/mailman/listinfo/devel > <https://lists.openvz.org/mailman/listinfo/devel> _______________________________________________ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
