From: Bharath SM <bharathsm.hsk@gmail.com>
To: Steve French <smfrench@gmail.com>
Cc: pc@cjr.nz, sfrench@samba.org, nspmangalore@gmail.com,
tom@talpey.com, linux-cifs@vger.kernel.org,
bharathsm@microsoft.com,
ronnie sahlberg <ronniesahlberg@gmail.com>
Subject: Re: [PATCH] cifs: prevent updating file size from server if we have a read/write lease
Date: Thu, 29 Feb 2024 23:23:06 +0530 [thread overview]
Message-ID: <CAGypqWx8x=q_srJLp7w1ygn0kgfTD8s_VP3wPyqp6mh3APoO6g@mail.gmail.com> (raw)
In-Reply-To: <CAGypqWx9JwO5nz-S+Yr8kw3UBsZPk5n0hiwzGa632pm_f1zpWA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]
Attached updated patch.
On Thu, Feb 29, 2024 at 11:22 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
>
> minor update to resolve conflicts.
> And Cc: stable@vger.kernel.org
>
> On Wed, Feb 28, 2024 at 3:57 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
> >
> > Attached updated patch to have this fix only for calls from readdir
> > i.e cifs_prime_dcache.
> >
> > On Mon, Feb 26, 2024 at 10:44 AM Steve French <smfrench@gmail.com> wrote:
> > >
> > > My only worry is that perhaps we should make it more narrow (ie only
> > > when called from readdir ie cifs_prime_dcache() rather than also
> > > never updating it on query_info calls)
> > >
> > > On Sun, Feb 25, 2024 at 10:50 PM Bharath SM <bharathsm.hsk@gmail.com> wrote:
> > > >
> > > > In cases of large directories, the readdir operation may span multiple
> > > > round trips to retrieve contents. This introduces a potential race
> > > > condition in case of concurrent write and readdir operations. If the
> > > > readdir operation initiates before a write has been processed by the
> > > > server, it may update the file size attribute to an older value.
> > > > Address this issue by avoiding file size updates from server when a
> > > > read/write lease.
> > > >
> > > > Scenario:
> > > > 1) process1: open dir xyz
> > > > 2) process1: readdir instance 1 on xyz
> > > > 3) process2: create file.txt for write
> > > > 4) process2: write x bytes to file.txt
> > > > 5) process2: close file.txt
> > > > 6) process2: open file.txt for read
> > > > 7) process1: readdir 2 - overwrites file.txt inode size to 0
> > > > 8) process2: read contents of file.txt - bug, short read with 0 bytes
> > > >
> > > > Signed-off-by: Bharath SM <bharathsm@microsoft.com>
> > > > ---
> > > > fs/smb/client/file.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> > > > index f2db4a1f81ad..e742d0d0e579 100644
> > > > --- a/fs/smb/client/file.c
> > > > +++ b/fs/smb/client/file.c
> > > > @@ -2952,7 +2952,8 @@ bool is_size_safe_to_change(struct cifsInodeInfo *cifsInode, __u64 end_of_file)
> > > > if (!cifsInode)
> > > > return true;
> > > >
> > > > - if (is_inode_writable(cifsInode)) {
> > > > + if (is_inode_writable(cifsInode) ||
> > > > + ((cifsInode->oplock & CIFS_CACHE_RW_FLG) != 0)) {
> > > > /* This inode is open for write at least once */
> > > > struct cifs_sb_info *cifs_sb;
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
[-- Attachment #2: 0001-cifs-prevent-updating-file-size-from-server-if-we-ha.patch --]
[-- Type: application/octet-stream, Size: 6389 bytes --]
From 5474dc566eb0a90f1eeaff801efe6dc5716b0417 Mon Sep 17 00:00:00 2001
From: Bharath SM <bharathsm@microsoft.com>
Date: Thu, 29 Feb 2024 23:09:52 +0530
Subject: [PATCH] cifs: prevent updating file size from server if we have a
read/write lease
In cases of large directories, the readdir operation may span multiple
round trips to retrieve contents. This introduces a potential race
condition in case of concurrent write and readdir operations. If the
readdir operation initiates before a write has been processed by the
server, it may update the file size attribute to an older value.
Address this issue by avoiding file size updates from readdir when we
have read/write lease.
Scenario:
1) process1: open dir xyz
2) process1: readdir instance 1 on xyz
3) process2: create file.txt for write
4) process2: write x bytes to file.txt
5) process2: close file.txt
6) process2: open file.txt for read
7) process1: readdir 2 - overwrites file.txt inode size to 0
8) process2: read contents of file.txt - bug, short read with 0 bytes
Cc: stable@vger.kernel.org
Signed-off-by: Bharath SM <bharathsm@microsoft.com>
---
fs/smb/client/cifsproto.h | 6 ++++--
fs/smb/client/file.c | 8 +++++---
fs/smb/client/inode.c | 13 +++++++------
fs/smb/client/readdir.c | 2 +-
4 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index a841bf4967fa..58cfbd450a55 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -144,7 +144,8 @@ extern int cifs_reconnect(struct TCP_Server_Info *server,
extern int checkSMB(char *buf, unsigned int len, struct TCP_Server_Info *srvr);
extern bool is_valid_oplock_break(char *, struct TCP_Server_Info *);
extern bool backup_cred(struct cifs_sb_info *);
-extern bool is_size_safe_to_change(struct cifsInodeInfo *, __u64 eof);
+extern bool is_size_safe_to_change(struct cifsInodeInfo *cifsInode, __u64 eof,
+ bool from_readdir);
extern void cifs_update_eof(struct cifsInodeInfo *cifsi, loff_t offset,
unsigned int bytes_written);
extern struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *, int);
@@ -201,7 +202,8 @@ extern void cifs_unix_basic_to_fattr(struct cifs_fattr *fattr,
struct cifs_sb_info *cifs_sb);
extern void cifs_dir_info_to_fattr(struct cifs_fattr *, FILE_DIRECTORY_INFO *,
struct cifs_sb_info *);
-extern int cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr);
+extern int cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr,
+ bool from_readdir);
extern struct inode *cifs_iget(struct super_block *sb,
struct cifs_fattr *fattr);
diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
index f391c9b803d8..9cff5f7dc062 100644
--- a/fs/smb/client/file.c
+++ b/fs/smb/client/file.c
@@ -329,7 +329,7 @@ int cifs_posix_open(const char *full_path, struct inode **pinode,
}
} else {
cifs_revalidate_mapping(*pinode);
- rc = cifs_fattr_to_inode(*pinode, &fattr);
+ rc = cifs_fattr_to_inode(*pinode, &fattr, false);
}
posix_open_ret:
@@ -4738,12 +4738,14 @@ static int is_inode_writable(struct cifsInodeInfo *cifs_inode)
refreshing the inode only on increases in the file size
but this is tricky to do without racing with writebehind
page caching in the current Linux kernel design */
-bool is_size_safe_to_change(struct cifsInodeInfo *cifsInode, __u64 end_of_file)
+bool is_size_safe_to_change(struct cifsInodeInfo *cifsInode, __u64 end_of_file,
+ bool from_readdir)
{
if (!cifsInode)
return true;
- if (is_inode_writable(cifsInode)) {
+ if (is_inode_writable(cifsInode) ||
+ ((cifsInode->oplock & CIFS_CACHE_RW_FLG) != 0 && from_readdir)) {
/* This inode is open for write at least once */
struct cifs_sb_info *cifs_sb;
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index d02f8ba29cb5..7f28edf4b20f 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -147,7 +147,8 @@ cifs_nlink_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
/* populate an inode with info from a cifs_fattr struct */
int
-cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
+cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr,
+ bool from_readdir)
{
struct cifsInodeInfo *cifs_i = CIFS_I(inode);
struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
@@ -199,7 +200,7 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
* Can't safely change the file size here if the client is writing to
* it due to potential races.
*/
- if (is_size_safe_to_change(cifs_i, fattr->cf_eof)) {
+ if (is_size_safe_to_change(cifs_i, fattr->cf_eof, from_readdir)) {
i_size_write(inode, fattr->cf_eof);
/*
@@ -368,7 +369,7 @@ static int update_inode_info(struct super_block *sb,
CIFS_I(*inode)->time = 0; /* force reval */
return -ESTALE;
}
- return cifs_fattr_to_inode(*inode, fattr);
+ return cifs_fattr_to_inode(*inode, fattr, false);
}
#ifdef CONFIG_CIFS_ALLOW_INSECURE_LEGACY
@@ -403,7 +404,7 @@ cifs_get_file_info_unix(struct file *filp)
} else
goto cifs_gfiunix_out;
- rc = cifs_fattr_to_inode(inode, &fattr);
+ rc = cifs_fattr_to_inode(inode, &fattr, false);
cifs_gfiunix_out:
free_xid(xid);
@@ -934,7 +935,7 @@ cifs_get_file_info(struct file *filp)
fattr.cf_uniqueid = CIFS_I(inode)->uniqueid;
fattr.cf_flags |= CIFS_FATTR_NEED_REVAL;
/* if filetype is different, return error */
- rc = cifs_fattr_to_inode(inode, &fattr);
+ rc = cifs_fattr_to_inode(inode, &fattr, false);
cgfi_exit:
cifs_free_open_info(&data);
free_xid(xid);
@@ -1491,7 +1492,7 @@ cifs_iget(struct super_block *sb, struct cifs_fattr *fattr)
}
/* can't fail - see cifs_find_inode() */
- cifs_fattr_to_inode(inode, fattr);
+ cifs_fattr_to_inode(inode, fattr, false);
if (sb->s_flags & SB_NOATIME)
inode->i_flags |= S_NOATIME | S_NOCMTIME;
if (inode->i_state & I_NEW) {
diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
index b520eea7bfce..132ae7d884a9 100644
--- a/fs/smb/client/readdir.c
+++ b/fs/smb/client/readdir.c
@@ -148,7 +148,7 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
rc = -ESTALE;
}
}
- if (!rc && !cifs_fattr_to_inode(inode, fattr)) {
+ if (!rc && !cifs_fattr_to_inode(inode, fattr, true)) {
dput(dentry);
return;
}
--
2.34.1
next prev parent reply other threads:[~2024-02-29 17:53 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-26 4:50 [PATCH] cifs: prevent updating file size from server if we have a read/write lease Bharath SM
2024-02-26 5:13 ` Steve French
2024-02-28 10:27 ` Bharath SM
2024-02-29 17:52 ` Bharath SM
2024-02-29 17:53 ` Bharath SM [this message]
2024-03-05 9:40 ` Shyam Prasad N
2024-03-11 0:32 ` Steve French
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGypqWx8x=q_srJLp7w1ygn0kgfTD8s_VP3wPyqp6mh3APoO6g@mail.gmail.com' \
--to=bharathsm.hsk@gmail.com \
--cc=bharathsm@microsoft.com \
--cc=linux-cifs@vger.kernel.org \
--cc=nspmangalore@gmail.com \
--cc=pc@cjr.nz \
--cc=ronniesahlberg@gmail.com \
--cc=sfrench@samba.org \
--cc=smfrench@gmail.com \
--cc=tom@talpey.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.