search for: balloon_page_dequeue

On Mon, Dec 28, 2015 at 08:35:13AM +0900, Minchan Kim wrote: > In balloon_page_dequeue, pages_lock should cover the loop > (ie, list_for_each_entry_safe). Otherwise, the cursor page could > be isolated by compaction and then list_del by isolation could > poison the page->lru.{prev,next} so the loop finally could > access wrong address like this. This patch fixes the bu...

On Mon, Dec 28, 2015 at 08:35:13AM +0900, Minchan Kim wrote: > In balloon_page_dequeue, pages_lock should cover the loop > (ie, list_for_each_entry_safe). Otherwise, the cursor page could > be isolated by compaction and then list_del by isolation could > poison the page->lru.{prev,next} so the loop finally could > access wrong address like this. This patch fixes the bu...

On Fri, Jan 08, 2016 at 02:56:14PM -0500, Rafael Aquini wrote: > On Fri, Jan 01, 2016 at 11:36:13AM +0200, Michael S. Tsirkin wrote: > > On Mon, Dec 28, 2015 at 08:35:13AM +0900, Minchan Kim wrote: > > > In balloon_page_dequeue, pages_lock should cover the loop > > > (ie, list_for_each_entry_safe). Otherwise, the cursor page could > > > be isolated by compaction and then list_del by isolation could > > > poison the page->lru.{prev,next} so the loop finally could > > > access wrong ad...

On Fri, Jan 08, 2016 at 02:56:14PM -0500, Rafael Aquini wrote: > On Fri, Jan 01, 2016 at 11:36:13AM +0200, Michael S. Tsirkin wrote: > > On Mon, Dec 28, 2015 at 08:35:13AM +0900, Minchan Kim wrote: > > > In balloon_page_dequeue, pages_lock should cover the loop > > > (ie, list_for_each_entry_safe). Otherwise, the cursor page could > > > be isolated by compaction and then list_del by isolation could > > > poison the page->lru.{prev,next} so the loop finally could > > > access wrong ad...

During my compaction-related stuff, I encountered a bug with ballooning. With repeated inflating and deflating cycle, guest memory( ie, cat /proc/meminfo | grep MemTotal) is decreased and couldn't be recovered. The reason is balloon_lock doesn't cover release_pages_balloon so struct virtio_balloon fields could be overwritten by race of fill_balloon(e,g, vb->*pfns could be critical).

During my compaction-related stuff, I encountered a bug with ballooning. With repeated inflating and deflating cycle, guest memory( ie, cat /proc/meminfo | grep MemTotal) is decreased and couldn't be recovered. The reason is balloon_lock doesn't cover release_pages_balloon so struct virtio_balloon fields could be overwritten by race of fill_balloon(e,g, vb->*pfns could be critical).

On Fri, Jan 01, 2016 at 11:36:13AM +0200, Michael S. Tsirkin wrote: > On Mon, Dec 28, 2015 at 08:35:13AM +0900, Minchan Kim wrote: > > In balloon_page_dequeue, pages_lock should cover the loop > > (ie, list_for_each_entry_safe). Otherwise, the cursor page could > > be isolated by compaction and then list_del by isolation could > > poison the page->lru.{prev,next} so the loop finally could > > access wrong address like this. Thi...

On Fri, Jan 01, 2016 at 11:36:13AM +0200, Michael S. Tsirkin wrote: > On Mon, Dec 28, 2015 at 08:35:13AM +0900, Minchan Kim wrote: > > In balloon_page_dequeue, pages_lock should cover the loop > > (ie, list_for_each_entry_safe). Otherwise, the cursor page could > > be isolated by compaction and then list_del by isolation could > > poison the page->lru.{prev,next} so the loop finally could > > access wrong address like this. Thi...

...; > >> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > >> + num = min_t(size_t, num, atomic64_read(&vb->num_pages)); > >> + for (num_pfns = 0; num_pfns < num; > >> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > >> page = balloon_page_dequeue(vb_dev_info); > > If balloon_page_dequeue() can be concurrently called by both host's request > > and guest's OOM event, is (!dequeued_page) test in balloon_page_dequeue() safe? > > > I'm not sure about the question. The "dequeue_page" is a local variabl...

...; > >> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > >> + num = min_t(size_t, num, atomic64_read(&vb->num_pages)); > >> + for (num_pfns = 0; num_pfns < num; > >> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > >> page = balloon_page_dequeue(vb_dev_info); > > If balloon_page_dequeue() can be concurrently called by both host's request > > and guest's OOM event, is (!dequeued_page) test in balloon_page_dequeue() safe? > > > I'm not sure about the question. The "dequeue_page" is a local variabl...

In balloon_page_dequeue, pages_lock should cover the loop (ie, list_for_each_entry_safe). Otherwise, the cursor page could be isolated by compaction and then list_del by isolation could poison the page->lru.{prev,next} so the loop finally could access wrong address like this. This patch fixes the bug. general protecti...

On Mon, Dec 28, 2015 at 08:35:13AM +0900, Minchan Kim wrote: > In balloon_page_dequeue, pages_lock should cover the loop > (ie, list_for_each_entry_safe). Otherwise, the cursor page could > be isolated by compaction and then list_del by isolation could > poison the page->lru.{prev,next} so the loop finally could > access wrong address like this. This patch fixes the bu...

...(vb->num_pfns = 0; vb->num_pfns < num; > - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > + num = min_t(size_t, num, atomic64_read(&vb->num_pages)); > + for (num_pfns = 0; num_pfns < num; > + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > page = balloon_page_dequeue(vb_dev_info); If balloon_page_dequeue() can be concurrently called by both host's request and guest's OOM event, is (!dequeued_page) test in balloon_page_dequeue() safe? Is such concurrency needed? > if (!page) > break; > - set_page_pfns(vb, vb->pfns + vb->num_pfns,...

...(vb->num_pfns = 0; vb->num_pfns < num; > - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > + num = min_t(size_t, num, atomic64_read(&vb->num_pages)); > + for (num_pfns = 0; num_pfns < num; > + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) { > page = balloon_page_dequeue(vb_dev_info); If balloon_page_dequeue() can be concurrently called by both host's request and guest's OOM event, is (!dequeued_page) test in balloon_page_dequeue() safe? Is such concurrency needed? > if (!page) > break; > - set_page_pfns(vb, vb->pfns + vb->num_pfns,...

Minchan Kim noticed that balloon_page_dequeue walks the pages list without holding the pages_lock. This can race e.g. with isolation, which has been reported to cause list corruption and crashes in leak_balloon. Page can also in theory get freed before it's locked, corrupting memory. To fix, make sure list accesses are done under lock, an...

Minchan Kim noticed that balloon_page_dequeue walks the pages list without holding the pages_lock. This can race e.g. with isolation, which has been reported to cause list corruption and crashes in leak_balloon. Page can also in theory get freed before it's locked, corrupting memory. To fix, make sure list accesses are done under lock, an...

...0644 > --- a/include/linux/balloon_compaction.h > +++ b/include/linux/balloon_compaction.h > @@ -64,6 +64,10 @@ extern struct page *balloon_page_alloc(void); > extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info, > struct page *page); > extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info); > +extern size_t balloon_page_list_enqueue(struct balloon_dev_info *b_dev_info, > + struct list_head *pages); > +extern size_t balloon_page_list_dequeue(struct balloon_dev_info *b_dev_info, > + struct list_head *pages, size_t n_req_...

...ages on a list, use balloon_page_list_enqueue instead. "To enqueue a list of pages" ? > */ > void balloon_page_enqueue(struct balloon_dev_info *b_dev_info, > struct page *page) > @@ -157,14 +156,24 @@ EXPORT_SYMBOL_GPL(balloon_page_enqueue); > > /* > * balloon_page_dequeue - removes a page from balloon's page list and > returns > - * the its address to allow the driver release the page. > + * its address to allow the driver to release the page. > * @b_dev_info: balloon device decriptor where we will grab a page from. > * > - * Driver...

...d my patch and picked up yours. Sorry about the noise. > > > > Also, there's another issue there I think: after isolation page could > > also get freed before we try to lock it. > > If a page was isolated, the page shouldn't stay b_dev_info->pages > list so balloon_page_dequeue cannot see the page. > Am I missing something? I mean without locks, as it is now. With either your or my patch in place, it's fine. -- MST

...d my patch and picked up yours. Sorry about the noise. > > > > Also, there's another issue there I think: after isolation page could > > also get freed before we try to lock it. > > If a page was isolated, the page shouldn't stay b_dev_info->pages > list so balloon_page_dequeue cannot see the page. > Am I missing something? I mean without locks, as it is now. With either your or my patch in place, it's fine. -- MST