| Message ID | 20200925150119.112016-2-colyli@suse.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Introduce sendpage_ok() to detect misused sendpage in network related drivers | expand |
On Fri, Sep 25, 2020 at 11:01:13PM +0800, Coly Li 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. > > 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(+) > > 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; Do you have one extra ' ' after "return" there? And this feels like a mm thing, why put it in net.h and not mm.h? thanks, greg k-h
On 2020/9/25 23:18, Greg KH wrote: > On Fri, Sep 25, 2020 at 11:01:13PM +0800, Coly Li 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. >> >> 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(+) >> >> 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; > > Do you have one extra ' ' after "return" there? It should be fixed in next version. > > And this feels like a mm thing, why put it in net.h and not mm.h? This check is specific for kernel_sendpage(), so I want to place it closer to where kernel_sendpage() is declared. And indeed there was similar discussion about why this helper is not in mm code in v5 series. Christoph supported to place sendpage_ok() in net.h, an uncompleted piece of his opinion was "It is not a mm bug, it is a networking quirk." Thanks. Coly Li
On Sat, Sep 26, 2020 at 09:28:03PM +0800, Coly Li wrote: > On 2020/9/25 23:18, Greg KH wrote: > > On Fri, Sep 25, 2020 at 11:01:13PM +0800, Coly Li 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. > >> > >> 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(+) > >> > >> 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; > > > > Do you have one extra ' ' after "return" there? > > It should be fixed in next version. > > > > > And this feels like a mm thing, why put it in net.h and not mm.h? > > This check is specific for kernel_sendpage(), so I want to place it > closer to where kernel_sendpage() is declared. > > And indeed there was similar discussion about why this helper is not in > mm code in v5 series. Christoph supported to place sendpage_ok() in > net.h, an uncompleted piece of his opinion was "It is not a mm bug, it > is a networking quirk." Ah, nevermind then, sorry for the noise :) greg k-h
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(+)