| Message ID | 20240226045010.30908-1-bharathsm@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: prevent updating file size from server if we have a read/write lease | expand |
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
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
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
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
On Thu, Feb 29, 2024 at 11:23 PM Bharath SM <bharathsm.hsk@gmail.com> wrote: > > 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 Changes look mostly good. >> 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; Why not use CIFS_CACHE_READ(cifsInode) || CIFS_CACHE_WRITE(cifsInode) instead of just checking the flag? That will cover other cache modes where attrs cannot change outside the client. Once this change is done, you can add my RB. Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
On Tue, Mar 5, 2024 at 3:41 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Thu, Feb 29, 2024 at 11:23 PM Bharath SM <bharathsm.hsk@gmail.com> wrote: > > > > 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 > > Changes look mostly good. > > >> 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; > > Why not use CIFS_CACHE_READ(cifsInode) || CIFS_CACHE_WRITE(cifsInode) > instead of just checking the flag? > That will cover other cache modes where attrs cannot change outside the client. I think Bharath's version is safer since if you did CIFS_CACHE_READ check you would be adding a case where you mounted read-only (and in that case it looks confusing to allow write size to be updated locally). It may also be a little risky to allow this when no you were not able to get a lease from the server and are mounting in "singleclient" mode.
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;
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(-)