Re: [PATCH v2] vfs: fix link vs. rename race

[Miklos Szeredi <miklos@szeredi.hu>]

On Tue, 13 Sept 2022 at 10:02, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Sep 13, 2022 at 7:38 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Tue, Sep 13, 2022 at 03:04:17AM +0100, Al Viro wrote:
> > > On Mon, Feb 21, 2022 at 09:20:02AM +0100, Miklos Szeredi wrote:
> > >
> > > [digging through the old piles of mail]
> > >
> > > Eyes-watering control flow in do_linkat() aside (it's bound to rot; too
> > > much of it won't get any regression testing and it's convoluted enough
> > > to break easily), the main problem I have with that is the DoS potential.
> > >
> > > You have a system-wide lock, and if it's stuck you'll get every damn
> > > rename(2) stuck as well.  Sure, having it taken only upon the race
> > > with rename() (or unlink(), for that matter) make it harder to get
> > > stuck with lock held, but that'll make the problem harder to reproduce
> > > and debug...
> >
> > FWIW, how much trouble would we have if link(2) would do the following?
> >
> >         find the parent of source
>
> Well, only if source is not AT_EMPTY_PATH

The problem is not AT_EMPTY_PATH, but a disconnected dentry gotten
through magic symlink.    Theoretically link(2) should work for those,
but I didn't try.

>
> >         lock it
> >         look the child up
> >         verify it's a non-directory
> >         bump child's i_nlink
> >                 all failure exits past that point decrement child's i_nlink
>
> No need to bump i_nlink.
> Sufficient to set I_LINKABLE.
> and clean it up on failure if i_nlink > 0.

Bumping i_nlink seems hackish enough, currently all i_nlink
modification is done by filesystem code.

Setting I_LINKABLE seems safe for filesystems that have tmpfile, not otherwise.

Thanks,
Miklos