All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Coly Li <colyli@suse.de>,
	linux-block@vger.kernel.org, linux-nvme@lists.infradead.org,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	stable <stable@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>,
	Christoph Hellwig <hch@lst.de>, Hannes Reinecke <hare@suse.de>,
	Jan Kara <jack@suse.com>, Jens Axboe <axboe@kernel.dk>,
	Mikhail Skorzhinskii <mskorzhinskiy@solarflare.com>,
	Philipp Reisner <philipp.reisner@linbit.com>,
	Sagi Grimberg <sagi@grimberg.me>,
	Vlastimil Babka <vbabka@suse.com>
Subject: Re: [PATCH v5 1/3] net: introduce helper sendpage_ok() in include/linux/net.h
Date: Mon, 17 Aug 2020 07:45:38 +0200	[thread overview]
Message-ID: <20200817054538.GA11705@lst.de> (raw)
In-Reply-To: <CAM_iQpUFtZdrhfUbuYYODNeSVqPOqx8mio6Znp6v3Q5iDZeyqg@mail.gmail.com>

On Sun, Aug 16, 2020 at 10:55:09AM -0700, Cong Wang wrote:
> On Sun, Aug 16, 2020 at 1:36 AM Coly Li <colyli@suse.de> wrote:
> >
> > 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.
> 
> Can we leave this helper to mm subsystem?
> 
> I know it is for sendpage, but its implementation is all about some
> mm details and its two callers do not belong to net subsystem either.
> 
> Think this in another way: who would fix it if it is buggy? I bet mm people
> should. ;)

No.  This is all about a really unusual imitation in sendpage, which
is pretty much unexpected.  In fact the best thing would be to make
sock_sendpage do the right thing and call sock_no_sendpage based
on this condition, so that driver writers don't have to worry at all.

WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de>
To: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>, Vlastimil Babka <vbabka@suse.com>,
	Sagi Grimberg <sagi@grimberg.me>,
	Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>,
	Mikhail Skorzhinskii <mskorzhinskiy@solarflare.com>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	stable <stable@vger.kernel.org>, Coly Li <colyli@suse.de>,
	linux-block@vger.kernel.org, linux-nvme@lists.infradead.org,
	Jan Kara <jack@suse.com>,
	Philipp Reisner <philipp.reisner@linbit.com>,
	Christoph Hellwig <hch@lst.de>, Hannes Reinecke <hare@suse.de>
Subject: Re: [PATCH v5 1/3] net: introduce helper sendpage_ok() in include/linux/net.h
Date: Mon, 17 Aug 2020 07:45:38 +0200	[thread overview]
Message-ID: <20200817054538.GA11705@lst.de> (raw)
In-Reply-To: <CAM_iQpUFtZdrhfUbuYYODNeSVqPOqx8mio6Znp6v3Q5iDZeyqg@mail.gmail.com>

On Sun, Aug 16, 2020 at 10:55:09AM -0700, Cong Wang wrote:
> On Sun, Aug 16, 2020 at 1:36 AM Coly Li <colyli@suse.de> wrote:
> >
> > 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.
> 
> Can we leave this helper to mm subsystem?
> 
> I know it is for sendpage, but its implementation is all about some
> mm details and its two callers do not belong to net subsystem either.
> 
> Think this in another way: who would fix it if it is buggy? I bet mm people
> should. ;)

No.  This is all about a really unusual imitation in sendpage, which
is pretty much unexpected.  In fact the best thing would be to make
sock_sendpage do the right thing and call sock_no_sendpage based
on this condition, so that driver writers don't have to worry at all.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

  reply	other threads:[~2020-08-17  5:45 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-16  7:15 [PATCH v5 1/3] net: introduce helper sendpage_ok() in include/linux/net.h Coly Li
2020-08-16  7:15 ` Coly Li
2020-08-16  7:15 ` [PATCH v5 2/3] nvme-tcp: check page by sendpage_ok() before calling kernel_sendpage() Coly Li
2020-08-16  7:15   ` Coly Li
2020-08-16  7:15 ` [PATCH v5 3/3] drbd: code cleanup by using sendpage_ok() to check page for kernel_sendpage() Coly Li
2020-08-16  7:15   ` Coly Li
2020-08-16 17:55 ` [PATCH v5 1/3] net: introduce helper sendpage_ok() in include/linux/net.h Cong Wang
2020-08-16 17:55   ` Cong Wang
2020-08-17  5:45   ` Christoph Hellwig [this message]
2020-08-17  5:45     ` Christoph Hellwig
2020-08-17 19:12     ` Cong Wang
2020-08-17 19:12       ` Cong Wang
2020-08-18  7:00       ` Christoph Hellwig
2020-08-18  7:00         ` Christoph Hellwig
  -- strict thread matches above, loose matches on Subject: below --
2020-08-16  7:08 Coly Li
2020-08-16  7:08 ` Coly Li
2020-08-18  8:08 ` Eric Dumazet
2020-08-18  8:08   ` Eric Dumazet
2020-08-18  8:21   ` Coly Li
2020-08-18  8:21     ` Coly Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200817054538.GA11705@lst.de \
    --to=hch@lst.de \
    --cc=axboe@kernel.dk \
    --cc=chaitanya.kulkarni@wdc.com \
    --cc=colyli@suse.de \
    --cc=hare@suse.de \
    --cc=jack@suse.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=mskorzhinskiy@solarflare.com \
    --cc=netdev@vger.kernel.org \
    --cc=philipp.reisner@linbit.com \
    --cc=sagi@grimberg.me \
    --cc=stable@vger.kernel.org \
    --cc=vbabka@suse.com \
    --cc=xiyou.wangcong@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.