linux-kernel - Re: Re: [PATCH] open hardlink on deferred close file return EINVAL

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: <20250318041821.24433-1-chunjie.zhu@cloud.com>
Date: Tue, 18 Mar 2025 04:18:21 +0000
From: Chunjie Zhu <chunjie.zhu@...ud.com>
To: smfrench@...il.com
Cc: chunjie.zhu@...ud.com,
	linux-cifs@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	lsahlber@...hat.com,
	pc@...guebit.com,
	ross.lagerwall@...ud.com,
	samba-technical@...ts.samba.org,
	sfrench@...ba.org,
	sprasad@...rosoft.com,
	tom@...pey.com
Subject: Re: Re: [PATCH] open hardlink on deferred close file return EINVAL

If we run 2 applications on a CIFS client machine, one application opens file A,
the other application opens file B which is hard link of file A, this issue would
happen, as well.

The purpose of this patch is to reduce the CIFS protocol network communication as
we can decide how to responsd to application at client side.

> It is fixed by running with leases disable (via mount parm), but
> wouldn't it be better to fix the error so apps don't break.  Ideas?
> 

Ideas,

Extend SMB SMB_COM_OPEN_ANDX and SMB_COM_NT_CREATE_ANDX messages, add file alias
into open or create request messages, an alias means a hard link of the original
file.

> On Mon, Mar 17, 2025 at 3:41=E2=80=AFAM Chunjie Zhu <chunjie.zhu@...ud.com>=
>  wrote:
> >
> > The following Python script results in unexpected behaviour when run on
> > a CIFS filesystem against a Windows Server:
> >
> >     # Create file
> >     fd =3D os.open('test', os.O_WRONLY|os.O_CREAT)
> >     os.write(fd, b'foo')
> >     os.close(fd)
> >
> >     # Open and close the file to leave a pending deferred close
> >     fd =3D os.open('test', os.O_RDONLY|os.O_DIRECT)
> >     os.close(fd)
> >
> >     # Try to open the file via a hard link
> >     os.link('test', 'new')
> >     newfd =3D os.open('new', os.O_RDONLY|os.O_DIRECT)
> >
> > The final open returns EINVAL due to the server returning
> > STATUS_INVALID_PARAMETER. The root cause of this is that the client
> > caches lease keys per inode, but the spec requires them to be related to
> > the filename which causes problems when hard links are involved:
> >
> > From MS-SMB2 section 3.3.5.9.11:
> >
> > "The server MUST attempt to locate a Lease by performing a lookup in the
> > LeaseTable.LeaseList using the LeaseKey in the
> > SMB2_CREATE_REQUEST_LEASE_V2 as the lookup key. If a lease is found,
> > Lease.FileDeleteOnClose is FALSE, and Lease.Filename does not match the
> > file name for the incoming request, the request MUST be failed with
> > STATUS_INVALID_PARAMETER"
> >
> > The client side can return EINVAL directly without invoking server
> > operations. This reduces client server network communication overhead.
> >
> > Signed-off-by: Chunjie Zhu <chunjie.zhu@...ud.com>
> > ---
> >  fs/smb/client/cifsproto.h |  2 ++
> >  fs/smb/client/file.c      | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 31 insertions(+)
> >
> > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> > index 260a6299bddb..b563c227792e 100644
> > --- a/fs/smb/client/cifsproto.h
> > +++ b/fs/smb/client/cifsproto.h
> > @@ -157,6 +157,8 @@ extern int cifs_get_writable_path(struct cifs_tcon *t=
> con, const char *name,
> >  extern struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *, b=
> ool);
> >  extern int cifs_get_readable_path(struct cifs_tcon *tcon, const char *na=
> me,
> >                                   struct cifsFileInfo **ret_file);
> > +extern int cifs_get_hardlink_path(struct cifs_tcon *tcon, struct inode *=
> inode,
> > +                                 struct file *file);
> >  extern unsigned int smbCalcSize(void *buf);
> >  extern int decode_negTokenInit(unsigned char *security_blob, int length,
> >                         struct TCP_Server_Info *server);
> > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> > index 4cbb5487bd8d..0a66cce6e0ff 100644
> > --- a/fs/smb/client/file.c
> > +++ b/fs/smb/client/file.c
> > @@ -751,6 +751,12 @@ int cifs_open(struct inode *inode, struct file *file=
> )
> >                 } else {
> >                         _cifsFileInfo_put(cfile, true, false);
> >                 }
> > +       } else {
> > +               /* hard link on the defeered close file */
> > +               rc =3D cifs_get_hardlink_path(tcon, inode, file);
> > +               if (rc) {
> > +                       goto out;
> > +               }
> >         }
> >
> >         if (server->oplocks)
> > @@ -2413,6 +2419,29 @@ cifs_get_readable_path(struct cifs_tcon *tcon, con=
> st char *name,
> >         return -ENOENT;
> >  }
> >
> > +int
> > +cifs_get_hardlink_path(struct cifs_tcon *tcon, struct inode *inode,
> > +                               struct file *file)
> > +{
> > +       struct cifsFileInfo *open_file =3D NULL;
> > +       struct cifsInodeInfo *cinode =3D CIFS_I(inode);
> > +       int rc =3D 0;
> > +
> > +       spin_lock(&tcon->open_file_lock);
> > +       spin_lock(&cinode->open_file_lock);
> > +
> > +       list_for_each_entry(open_file, &cinode->openFileList, flist) {
> > +               if (file->f_flags =3D=3D open_file->f_flags) {
> > +                       rc =3D -EINVAL;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       spin_unlock(&cinode->open_file_lock);
> > +       spin_unlock(&tcon->open_file_lock);
> > +       return rc;
> > +}
> > +
> >  void
> >  cifs_writedata_release(struct kref *refcount)
> >  {
> > --
> > 2.34.1
> >
> >
> 
> 
> --=20
> Thanks,
> 
> Steve
>