linux-kernel - Re: cifs - Race between IP address change and sget()?

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAH2r5msv0r1-bXJKdN-ansMnpay0+Aw9FP5us5zHrhp3adzV=Q@mail.gmail.com>
Date:   Mon, 20 Apr 2020 21:29:44 -0500
From:   Steve French <smfrench@...il.com>
To:     Jeff Layton <jlayton@...hat.com>
Cc:     David Howells <dhowells@...hat.com>, Paulo Alcantara <pc@....nz>,
        Al Viro <viro@...iv.linux.org.uk>,
        linux-nfs <linux-nfs@...r.kernel.org>,
        CIFS <linux-cifs@...r.kernel.org>, linux-afs@...ts.infradead.org,
        ceph-devel@...r.kernel.org, keyrings@...r.kernel.org,
        Network Development <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Florian Weimer <fweimer@...hat.com>
Subject: Re: cifs - Race between IP address change and sget()?

On Mon, Apr 20, 2020 at 5:30 PM Jeff Layton <jlayton@...hat.com> wrote:
>
> On Mon, 2020-04-20 at 23:14 +0100, David Howells wrote:
> > Paulo Alcantara <pc@....nz> wrote:
> >
> > > > > > What happens if the IP address the superblock is going to changes, then
> > > > > > another mount is made back to the original IP address?  Does the second
> > > > > > mount just pick the original superblock?
> > > > >
> > > > > It is going to transparently reconnect to the new ip address, SMB share,
> > > > > and cifs superblock is kept unchanged.  We, however, update internal
> > > > > TCP_Server_Info structure to reflect new destination ip address.
> > > > >
> > > > > For the second mount, since the hostname (extracted out of the UNC path
> > > > > at mount time) resolves to a new ip address and that address was saved
> > > > > earlier in TCP_Server_Info structure during reconnect, we will end up
> > > > > reusing same cifs superblock as per fs/cifs/connect.c:cifs_match_super().
> > > >
> > > > Would that be a bug?
> > >
> > > Probably.
> > >
> > > I'm not sure how that code is supposed to work, TBH.
> >
> > Hmmm...  I think there may be a race here then - but I'm not sure it can be
> > avoided or if it matters.
> >
> > Since the address is part of the primary key to sget() for cifs, changing the
> > IP address will change the primary key.  Jeff tells me that this is governed
> > by a spinlock taken by cifs_match_super().  However, sget() may be busy
> > attaching a new mount to the old superblock under the sb_lock core vfs lock,
> > having already found a match.
> >
>
> Not exactly. Both places that match TCP_Server_Info objects by address
> hold the cifs_tcp_ses_lock. The address looks like it gets changed in
> reconn_set_ipaddr, and the lock is not currently taken there, AFAICT. I
> think it probably should be (at least around the cifs_convert_address
> call).
>
> > Should the change of parameters made by cifs be effected with sb_lock held to
> > try and avoid ending up using the wrong superblock?
> >
> > However, because the TCP_Server_Info is apparently updated, it looks like my
> > original concern is not actually a problem (the idea that if a mounted server
> > changes its IP address and then a new server comes online at the old IP
> > address, it might end up sharing superblocks because the IP address is part of
> > the key).
> >
>
> I'm not sure we should concern ourselves with much more than just not
> allowing addresses to change while matching/searching. If you're
> standing up new servers at old addresses while you still have clients
> are migrating, then you are probably Doing it Wrong.

Yes.   The most important thing is to support the reasonably
common scenario where the server's IP address changes (there are
fancier ways to handle this gracefully e.g. the "Witness Protocol" feature
of SMB3 mounts, but that is not always supported by servers and we
still need to add Witness protocol to clients - but current code allowing
SMB3 server IP address to change has helped some customers out)

-- 
Thanks,

Steve