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] [thread-next>] [day] [month] [year] [list]

Message-ID: <194431215.23515823.1587432599559.JavaMail.zimbra@redhat.com>
Date:   Mon, 20 Apr 2020 21:29:59 -0400 (EDT)
From:   Ronnie Sahlberg <lsahlber@...hat.com>
To:     Jeff Layton <jlayton@...hat.com>
Cc:     David Howells <dhowells@...hat.com>, Paulo Alcantara <pc@....nz>,
        viro@...iv.linux.org.uk, Steve French <smfrench@...il.com>,
        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>, fweimer@...hat.com
Subject: Re: cifs - Race between IP address change and sget()?





----- Original Message -----
> From: "Jeff Layton" <jlayton@...hat.com>
> To: "David Howells" <dhowells@...hat.com>, "Paulo Alcantara" <pc@....nz>
> Cc: viro@...iv.linux.org.uk, "Steve French" <smfrench@...il.com>, "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>, fweimer@...hat.com
> Sent: Tuesday, 21 April, 2020 8:30:37 AM
> Subject: Re: cifs - Race between IP address change and sget()?
> 
> 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).

I think you are right. We need the spinlock around this call too.
I will send a patch to the list to add this.

> 
> > 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.

Agree. That is a migration process issue and not something we can/should
try to address in cifs.ko.



> 
> --
> Jeff Layton <jlayton@...hat.com>
> 
>