linux-kernel - Re: [PATCH] cifs: Fix reacquisition of volume cookie on still-live connection

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: <1a94a15e6863d3844f0bcb58b7b1e17a@manguebit.com>
Date: Wed, 17 Apr 2024 11:09:27 -0300
From: Paulo Alcantara <pc@...guebit.com>
To: David Howells <dhowells@...hat.com>
Cc: dhowells@...hat.com, Steve French <sfrench@...ba.org>, Shyam Prasad N
 <sprasad@...rosoft.com>, linux-cifs@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cifs: Fix reacquisition of volume cookie on still-live
 connection

David Howells <dhowells@...hat.com> writes:

> Paulo Alcantara <pc@...guebit.com> wrote:
>
>> Can't we just move the cookie acquisition to cifs_get_tcon() before it
>> gets added to list @ses->tcon_list.  This way we'll guarantee that the
>> cookie is set only once for the new tcon.
>
> cifs_get_tcon() is used from more than one place and I'm not sure the second
> place (__cifs_construct_tcon()) actually wants a cookie.  I'm not sure what
> that path is for.

__cifs_construct_tcon() is used for creating sessions and tcons under
multiuser mounts.  Whenever an user accesses a multiuser mount and the
client can't find a credential for it, a new session and tcon will be
created for the user accessing the mount -- new accesses from same user
will end up reusing the created session and tcon.

And yes, I don't think we'll need a cookie for those tcons as the client
seems to get the fscache cookie from master tcon (the one created from
mount credentials).

> Could all the (re)setting up being done in cifs_mount_get_tcon() be
> pushed back into cifs_get_tcon()?

AFAICT, yes.  I'd need to look into it to make sure that's safe.

>> Besides, do we want to share a tcon with two different superblocks that
>> have 'fsc' and 'nofsc', respectively?  If not, it would be better to fix
>> match_tcon() as well to handle such case.
>
> Maybe?  What does a tcon *actually* represent?  I note that in
> cifs_match_super(), it's not the only criterion matched upon - so you can, at
> least in apparent theory, get different superblocks for the same tcon anyway.

tcon simply represents a tree connected SMB share.  It can be either an
IPC share (\\srv\IPC$) or the actual share (\\srv\share) we're accessing
the files from.

Consider the following example where a tcon is reused from different
CIFS superblocks:

  mount.cifs //srv/share /mnt/1 -o ${opts} # new super, new tcon
  mount.cifs //srv/share/dir /mnt/2 -o ${opts} # new super, reused tcon

So, /mnt/1/dir/foo and /mnt/2/foo will lead to different inodes.

The two mounts are accessing the same tcon (\\srv\share) but the new
superblock was created because the prefix path "\dir" didn't match in
cifs_match_super().  Trust me, that's a very common scenario.

> This suggests that the tcon might not be the best place for the fscache volume
> cookie as you can have multiple inodes wishing to use the same file cookie if
> there are multiple mounts mounting the same tcon but, say, with different
> mount parameters.

We're not supposed to allow mounts with different parameters reusing
servers, sessions or tcons, so that should be no issue.

> I'm not sure what the right way around this is.  The root of the problem is
> coherency management.  If we make a change to an inode on one mounted
> superblock and this bounces a change notification over to the server that then
> pokes an inode in another mounted superblock on the same machine and causes it
> to be invalidated, you lose your local cache if both inodes refer to the same
> fscache cookie.

Yes, that could be a problem.  Perhaps placing the fscache cookie in the
superblock would be a way to go, so we can handle the different fscache
cookies for the superblocks that contain different prefix paths and
access same tcon.

> Remember: fscache does not do this for you!  It's just a facility by which
> which data can be stored and retrieved.  The netfs is responsible for telling
> it when to invalidate and handling coherency.

ACK.

> That said, it might be possible to time-share a cookie on cifs with leases,
> but the local superblocks would have to know about each other - in which case,
> why are they separate superblocks?

See above why they could be separate superblocks.