| Message ID | 20200818131227.37020-2-colyli@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce sendpage_ok() to detect misused sendpage in network related drivers | expand |
I think we should go for something simple like this instead: --- From 4867e158ee86ebd801b4c267e8f8a4a762a71343 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> 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 <hch@lst.de> --- 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);
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 <hch@lst.de> > 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 <hch@lst.de> > --- > 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); >
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.
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
On Wed, Aug 19, 2020 at 12:22:05PM +0800, Coly Li wrote: > 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. So given the feedback from Dave I suspect we should actually resurrect this series, sorry for the noise. And in this case I think we do need the warning in kernel_sendpage.
On 2020/9/23 16:43, Christoph Hellwig wrote: > On Wed, Aug 19, 2020 at 12:22:05PM +0800, Coly Li wrote: >> 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. > > So given the feedback from Dave I suspect we should actually resurrect > this series, sorry for the noise. And in this case I think we do need > the warning in kernel_sendpage. > Copied, then I will post a v8 series, which adding a warning message in kernel_sendpage() if non-acceptible paage sent in. Coly Li
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 <linux/rcupdate.h> #include <linux/once.h> #include <linux/fs.h> +#include <linux/mm.h> #include <linux/sockptr.h> #include <uapi/linux/net.h> @@ -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,
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 <colyli@suse.de> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Hannes Reinecke <hare@suse.de> Cc: Jan Kara <jack@suse.com> Cc: Jens Axboe <axboe@kernel.dk> Cc: Mikhail Skorzhinskii <mskorzhinskiy@solarflare.com> Cc: Philipp Reisner <philipp.reisner@linbit.com> Cc: Sagi Grimberg <sagi@grimberg.me> Cc: Vlastimil Babka <vbabka@suse.com> Cc: stable@vger.kernel.org --- include/linux/net.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)