| Message ID | 9e65ec2e-5e22-4f65-7b92-ca2af0c555f3@cybernetics.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v4,1/9] dmapool: fix boundary comparison | expand |
On Mon, Nov 12, 2018 at 10:46:35AM -0500, Tony Battersby wrote: > Prevent a possible endless loop with DMAPOOL_DEBUG enabled if a buggy > driver corrupts DMA pool memory. > > Signed-off-by: Tony Battersby <tonyb@cybernetics.com> I like it! Also, here you're using blks_per_alloc in a way which isn't normally in the performance path, but might be with the right config options. With that, I withdraw my objection to the previous patch and Acked-by: Matthew Wilcox <willy@infradead.org> Andrew, can you funnel these in through your tree? If you'd rather not, I don't mind stuffing them into a git tree and asking Linus to pull for 4.21.
On 11/13/18 1:36 AM, Matthew Wilcox wrote: > On Mon, Nov 12, 2018 at 10:46:35AM -0500, Tony Battersby wrote: >> Prevent a possible endless loop with DMAPOOL_DEBUG enabled if a buggy >> driver corrupts DMA pool memory. >> >> Signed-off-by: Tony Battersby <tonyb@cybernetics.com> > I like it! Also, here you're using blks_per_alloc in a way which isn't > normally in the performance path, but might be with the right config > options. With that, I withdraw my objection to the previous patch and > > Acked-by: Matthew Wilcox <willy@infradead.org> > > Andrew, can you funnel these in through your tree? If you'd rather not, > I don't mind stuffing them into a git tree and asking Linus to pull > for 4.21. > No reply for 3 weeks, so adding Andrew Morton to recipient list. Andrew, I have 9 dmapool patches ready for merging in 4.21. See Matthew Wilcox's request above. Tony Battersby
On Tue, 4 Dec 2018 11:22:34 -0500 Tony Battersby <tonyb@cybernetics.com> wrote: > On 11/13/18 1:36 AM, Matthew Wilcox wrote: > > On Mon, Nov 12, 2018 at 10:46:35AM -0500, Tony Battersby wrote: > >> Prevent a possible endless loop with DMAPOOL_DEBUG enabled if a buggy > >> driver corrupts DMA pool memory. > >> > >> Signed-off-by: Tony Battersby <tonyb@cybernetics.com> > > I like it! Also, here you're using blks_per_alloc in a way which isn't > > normally in the performance path, but might be with the right config > > options. With that, I withdraw my objection to the previous patch and > > > > Acked-by: Matthew Wilcox <willy@infradead.org> > > > > Andrew, can you funnel these in through your tree? If you'd rather not, > > I don't mind stuffing them into a git tree and asking Linus to pull > > for 4.21. > > > No reply for 3 weeks, so adding Andrew Morton to recipient list. > > Andrew, I have 9 dmapool patches ready for merging in 4.21. See Matthew > Wilcox's request above. > I'll take a look, but I see that this v4 series has several review comments from Matthew which remain unresponded to. Please attend to that. Also, Andy had issues with the v2 series so it would be good to hear an update from him?
On Tue, Dec 04, 2018 at 12:14:43PM -0800, Andrew Morton wrote: > On Tue, 4 Dec 2018 11:22:34 -0500 Tony Battersby <tonyb@cybernetics.com> wrote: > > > On 11/13/18 1:36 AM, Matthew Wilcox wrote: > > > On Mon, Nov 12, 2018 at 10:46:35AM -0500, Tony Battersby wrote: > > >> Prevent a possible endless loop with DMAPOOL_DEBUG enabled if a buggy > > >> driver corrupts DMA pool memory. > > >> > > >> Signed-off-by: Tony Battersby <tonyb@cybernetics.com> > > > I like it! Also, here you're using blks_per_alloc in a way which isn't > > > normally in the performance path, but might be with the right config > > > options. With that, I withdraw my objection to the previous patch and > > > > > > Acked-by: Matthew Wilcox <willy@infradead.org> > > > > > > Andrew, can you funnel these in through your tree? If you'd rather not, > > > I don't mind stuffing them into a git tree and asking Linus to pull > > > for 4.21. > > > > > No reply for 3 weeks, so adding Andrew Morton to recipient list. > > > > Andrew, I have 9 dmapool patches ready for merging in 4.21. See Matthew > > Wilcox's request above. > > > > I'll take a look, but I see that this v4 series has several review > comments from Matthew which remain unresponded to. Please attend to > that. I only had a review comment on 8/9, which I then withdrew during my review of patch 9/9. Unless I missed something during my re-review of my responses? > Also, Andy had issues with the v2 series so it would be good to hear an > update from him? Certainly.
On Tue, 4 Dec 2018 12:18:01 -0800 Matthew Wilcox <willy@infradead.org> wrote: > On Tue, Dec 04, 2018 at 12:14:43PM -0800, Andrew Morton wrote: > > On Tue, 4 Dec 2018 11:22:34 -0500 Tony Battersby <tonyb@cybernetics.com> wrote: > > > > > On 11/13/18 1:36 AM, Matthew Wilcox wrote: > > > > On Mon, Nov 12, 2018 at 10:46:35AM -0500, Tony Battersby wrote: > > > >> Prevent a possible endless loop with DMAPOOL_DEBUG enabled if a buggy > > > >> driver corrupts DMA pool memory. > > > >> > > > >> Signed-off-by: Tony Battersby <tonyb@cybernetics.com> > > > > I like it! Also, here you're using blks_per_alloc in a way which isn't > > > > normally in the performance path, but might be with the right config > > > > options. With that, I withdraw my objection to the previous patch and > > > > > > > > Acked-by: Matthew Wilcox <willy@infradead.org> > > > > > > > > Andrew, can you funnel these in through your tree? If you'd rather not, > > > > I don't mind stuffing them into a git tree and asking Linus to pull > > > > for 4.21. > > > > > > > No reply for 3 weeks, so adding Andrew Morton to recipient list. > > > > > > Andrew, I have 9 dmapool patches ready for merging in 4.21. See Matthew > > > Wilcox's request above. > > > > > > > I'll take a look, but I see that this v4 series has several review > > comments from Matthew which remain unresponded to. Please attend to > > that. > > I only had a review comment on 8/9, which I then withdrew during my review > of patch 9/9. Unless I missed something during my re-review of my responses? And in 0/9, that 1.3MB allocation. Maybe it's using kvmalloc, I didn't look.
On Tue, Dec 4, 2018 at 10:18 PM Matthew Wilcox <willy@infradead.org> wrote: > On Tue, Dec 04, 2018 at 12:14:43PM -0800, Andrew Morton wrote: > > Also, Andy had issues with the v2 series so it would be good to hear an > > update from him? > > Certainly. Hmm... I certainly forgot what was long time ago. If I _was_ in Cc list and didn't comment, I'm fine with it.
On Tue, Dec 04, 2018 at 12:28:54PM -0800, Andrew Morton wrote: > On Tue, 4 Dec 2018 12:18:01 -0800 Matthew Wilcox <willy@infradead.org> wrote: > > I only had a review comment on 8/9, which I then withdrew during my review > > of patch 9/9. Unless I missed something during my re-review of my responses? > > And in 0/9, that 1.3MB allocation. > > Maybe it's using kvmalloc, I didn't look. Oh! That's the mptsas driver doing something utterly awful. Not the fault of this patchset, in any way.
On 12/4/18 3:30 PM, Andy Shevchenko wrote: > On Tue, Dec 4, 2018 at 10:18 PM Matthew Wilcox <willy@infradead.org> wrote: >> On Tue, Dec 04, 2018 at 12:14:43PM -0800, Andrew Morton wrote: >>> Also, Andy had issues with the v2 series so it would be good to hear an >>> update from him? >> Certainly. > Hmm... I certainly forgot what was long time ago. > If I _was_ in Cc list and didn't comment, I'm fine with it. > v4 of the patchset is the same as v3 but with the last patch dropped. Andy had only one minor comment on v3 about the use of division in patch #8, to which I replied. That was back on August 8. Tony
On Tue, Dec 4, 2018 at 11:26 PM Tony Battersby <tonyb@cybernetics.com> wrote: > > On 12/4/18 3:30 PM, Andy Shevchenko wrote: > > On Tue, Dec 4, 2018 at 10:18 PM Matthew Wilcox <willy@infradead.org> wrote: > >> On Tue, Dec 04, 2018 at 12:14:43PM -0800, Andrew Morton wrote: > >>> Also, Andy had issues with the v2 series so it would be good to hear an > >>> update from him? > >> Certainly. > > Hmm... I certainly forgot what was long time ago. > > If I _was_ in Cc list and didn't comment, I'm fine with it. > > > v4 of the patchset is the same as v3 but with the last patch dropped. > Andy had only one minor comment on v3 about the use of division in patch > #8, to which I replied. That was back on August 8. Seems I'm fine with the last version then.
--- linux/mm/dmapool.c.orig 2018-08-06 17:52:53.000000000 -0400 +++ linux/mm/dmapool.c 2018-08-06 17:53:31.000000000 -0400 @@ -454,17 +454,39 @@ void dma_pool_free(struct dma_pool *pool { void *page_vaddr = vaddr - offset; unsigned int chain = page->dma_free_off; + unsigned int free_blks = 0; + while (chain < pool->allocation) { - if (chain != offset) { - chain = *(int *)(page_vaddr + chain); - continue; + if (unlikely(chain == offset)) { + spin_unlock_irqrestore(&pool->lock, flags); + dev_err(pool->dev, + "dma_pool_free %s, dma %pad already free\n", + pool->name, &dma); + return; + } + + /* + * A buggy driver could corrupt the freelist by + * use-after-free, buffer overflow, etc. Besides + * checking for corruption, this also prevents an + * endless loop in case corruption causes a circular + * loop in the freelist. + */ + if (unlikely(++free_blks + page->dma_in_use > + pool->blks_per_alloc)) { + freelist_corrupt: + spin_unlock_irqrestore(&pool->lock, flags); + dev_err(pool->dev, + "dma_pool_free %s, freelist corrupted\n", + pool->name); + return; } - spin_unlock_irqrestore(&pool->lock, flags); - dev_err(pool->dev, - "dma_pool_free %s, dma %pad already free\n", - pool->name, &dma); - return; + + chain = *(int *)(page_vaddr + chain); } + if (unlikely(free_blks + page->dma_in_use != + pool->blks_per_alloc)) + goto freelist_corrupt; } memset(vaddr, POOL_POISON_FREED, pool->size); #endif
Prevent a possible endless loop with DMAPOOL_DEBUG enabled if a buggy driver corrupts DMA pool memory. Signed-off-by: Tony Battersby <tonyb@cybernetics.com> ---