linux-kernel - Re: [PATCH v3 3/3] block: avoid to call .bi_end

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 28 Apr 2016 11:58:28 -0400 (EDT)
From:	Mikulas Patocka <mpatocka@...hat.com>
To:	Ming Lei <ming.lei@...onical.com>
cc:	Jens Axboe <axboe@...com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-block@...r.kernel.org, Christoph Hellwig <hch@...radead.org>,
	Btrfs BTRFS <linux-btrfs@...r.kernel.org>,
	Shaun Tancheff <shaun.tancheff@...gate.com>,
	Alan Cox <alan@...ux.intel.com>, Neil Brown <neilb@...e.de>,
	Liu Bo <bo.li.liu@...cle.com>, Jens Axboe <axboe@...nel.dk>
Subject: Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively



On Thu, 28 Apr 2016, Ming Lei wrote:

> Hi Mikulas,
> 
> On Thu, Apr 28, 2016 at 11:29 PM, Mikulas Patocka <mpatocka@...hat.com> wrote:
> >
> >
> > On Thu, 28 Apr 2016, Ming Lei wrote:
> >
> >> There were reports about heavy stack use by recursive calling
> >> .bi_end_io()([1][2][3]). For example, more than 16K stack is
> >> consumed in a single bio complete path[3], and in [2] stack
> >> overflow can be triggered if 20 nested dm-crypt is used.
> >>
> >> Also patches[1] [2] [3] were posted for addressing the issue,
> >> but never be merged. And the idea in these patches is basically
> >> similar, all serializes the recursive calling of .bi_end_io() by
> >> percpu list.
> >>
> >> This patch still takes the same idea, but uses bio_list to
> >> implement it, which turns out more simple and the code becomes
> >> more readable meantime.
> >>
> >> One corner case which wasn't covered before is that
> >> bi_endio() may be scheduled to run in process context(such
> >> as btrfs), and this patch just bypasses the optimizing for
> >> that case because one new context should have enough stack space,
> >> and this approach isn't capable of optimizing it too because
> >> there isn't easy way to get a per-task linked list head.
> >
> > Hi
> >
> > You could use preempt_disable() and then you could use per-cpu list even
> > in the process context.
> 
> Image why the .bi_end_io() is scheduled to process context, and the only
> workable/simple way I thought of is to use per-task list because it may sleep.

The bi_end_io callback should not sleep, even if it is called from the 
process context.

> Given new context should have enough stack and only btrfs has this kind of
> usage as far as I see, so don't think that is worth of the optimization.
> 
> Thanks,
> Ming

Mikulas