On Thu, Feb 29, 2024 at 11:23 PM Bharath SM <bharathsm.hsk@xxxxxxxxx> wrote:
>
> Attached updated patch.
>
> On Thu, Feb 29, 2024 at 11:22 PM Bharath SM <bharathsm.hsk@xxxxxxxxx> wrote:
> >
> > minor update to resolve conflicts.
> > And Cc: stable@xxxxxxxxxxxxxxx
> >
> > On Wed, Feb 28, 2024 at 3:57 PM Bharath SM <bharathsm.hsk@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxxxxxxxx>
--
Regards,
Shyam
