Re: [Qemu-devel] [PATCH] Fix hang with -L and symlink loop

[Evan Green]

On Thu, May 31, 2018 at 9:44 AM Peter Maydell <address@hidden> wrote:
>
> On 31 May 2018 at 17:32, Richard Henderson <address@hidden> wrote:
> > On 05/31/2018 08:26 AM, Peter Maydell wrote:
> >> On 30 May 2018 at 21:50, Richard Henderson <address@hidden> wrote:
> >>> On 05/29/2018 04:44 PM, Evan Green wrote:
> >>>> When using -L with Linux app emulation, there is an issue in
> >>>> init_paths where Qemu will get lost exploring a directory tree
> >>>> with a symlink loop in it. This causes Qemu to hang, and
> >>>> eventually consume all memory in the system.
> >>>>
> >>>> Qemu's code for pre-exploring the entire directory tree is both
> >>>> error-prone and slow. Instead, this changes uses faccessat, which
> >>>> both avoids the symlink loop (since the entire directory space isn't
> >>>> being explored up front), and likely speeds things up a bit.
> >>>>
> >>>> Partial credit goes to Richard Henderson, as it was only after staring
> >>>> at his patch [1] that I wrote mine.
> >>>>
> >>>> [1] https://patchwork.kernel.org/patch/9512083/
> >>>>
> >>>> Signed-off-by: Evan Green <address@hidden>
> >>>> ---
> >>>
> >>> I like this as an improvement on the current situation.
> >>> Perhaps folks do like this a bit better than my more invasive patch.
> >>
> >> It seems to me to have the same problem as your patch does,
> >> which is that we now have a file descriptor which belongs
> >> not to the guest but to QEMU, but we make no effort to
> >> hide it from the guest, and so for instance code like that
> >> xinetd "close every fd above 2" loop will break us.
> >
> > Yes.  I've thought about doing more explicit managing of guest fd's, but 
> > it's a
> > big job to re-organize that.
>
> I think my current opinion is that it's probably OK to have the
> "our internal fd is visible to the guest" problem for the case
> where the user specifies -L, at least for now. We're probably
> causing more problems by not allowing -L to point at trees with
> symlinks than by misbehaving if the application closes it.
> We should just leave ourselves a suitable TODO note or something.
>
> As I say, I prefer your patch to this one overall, if you
> wanted to address the minor issues from the last round of
> review and respin it.
>

That sounds good to me. Richard, would you mind CCing me on your next
spin of the patch? I'm not on the main list.
Thanks,
-Evan

> thanks
> -- PMM