[3/7] cifs: smb2_close_getattr should also update i_size

From: Shyam Prasad N <sprasad@microsoft.com>

SMB2 CLOSE command with SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB
flag set is already used by the code for SMB3+.
smb2_close_getattr is the function that uses this to
update the inode attributes.

However, we were skipping the EndOfFile info that's returned
by the server. There is a small chance that the file size
may have been changed in the small window between the client
sending the close request (thereby giving up lease if it had)
to the point that the server returns the response.

This change uses the field to update the inode size.
Also, it is a valid case for a zero AllocationSize to be returned
by the server for the file. We were discarding such values, thereby
resulting in stale i_blocks value. Fixed that here too.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/smb2ops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

On Sun, Jan 21, 2024 at 9:03 AM <nspmangalore@gmail.com> wrote:
>
> From: Shyam Prasad N <sprasad@microsoft.com>
>
> SMB2 CLOSE command with SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB
> flag set is already used by the code for SMB3+.
> smb2_close_getattr is the function that uses this to
> update the inode attributes.
>
> However, we were skipping the EndOfFile info that's returned
> by the server. There is a small chance that the file size
> may have been changed in the small window between the client
> sending the close request (thereby giving up lease if it had)
> to the point that the server returns the response.
>
> This change uses the field to update the inode size.
> Also, it is a valid case for a zero AllocationSize to be returned
> by the server for the file. We were discarding such values, thereby
> resulting in stale i_blocks value. Fixed that here too.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/smb2ops.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index d9553c2556a2..e23577584ed6 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -1433,9 +1433,9 @@ smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon,
>          * but instead 512 byte (2**9) size is required for
>          * calculating num blocks.
>          */
> -       if (le64_to_cpu(file_inf.AllocationSize) > 4096)
> -               inode->i_blocks =
> -                       (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9;
> +       inode->i_blocks = (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9;
> +
> +       inode->i_size = le64_to_cpu(file_inf.EndOfFile);
>
>         /* End of file and Attributes should not have to be updated on close */
>         spin_unlock(&inode->i_lock);
> --
> 2.34.1
>
Noticed a couple of things in other places in the code:
1. i_size_write() should be called instead of updating i_size directly.
2. There's a server_eof field that is generally maintained in sync
with i_size. Need to check how that needs to be done here.

I'll submit a v2 of this patch soon.

On Tue, Jan 23, 2024 at 12:52 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Sun, Jan 21, 2024 at 9:03 AM <nspmangalore@gmail.com> wrote:
> >
> > From: Shyam Prasad N <sprasad@microsoft.com>
> >
> > SMB2 CLOSE command with SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB
> > flag set is already used by the code for SMB3+.
> > smb2_close_getattr is the function that uses this to
> > update the inode attributes.
> >
> > However, we were skipping the EndOfFile info that's returned
> > by the server. There is a small chance that the file size
> > may have been changed in the small window between the client
> > sending the close request (thereby giving up lease if it had)
> > to the point that the server returns the response.
> >
> > This change uses the field to update the inode size.
> > Also, it is a valid case for a zero AllocationSize to be returned
> > by the server for the file. We were discarding such values, thereby
> > resulting in stale i_blocks value. Fixed that here too.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/smb/client/smb2ops.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > index d9553c2556a2..e23577584ed6 100644
> > --- a/fs/smb/client/smb2ops.c
> > +++ b/fs/smb/client/smb2ops.c
> > @@ -1433,9 +1433,9 @@ smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon,
> >          * but instead 512 byte (2**9) size is required for
> >          * calculating num blocks.
> >          */
> > -       if (le64_to_cpu(file_inf.AllocationSize) > 4096)
> > -               inode->i_blocks =
> > -                       (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9;
> > +       inode->i_blocks = (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9;
> > +
> > +       inode->i_size = le64_to_cpu(file_inf.EndOfFile);
> >
> >         /* End of file and Attributes should not have to be updated on close */
> >         spin_unlock(&inode->i_lock);
> > --
> > 2.34.1
> >
> Noticed a couple of things in other places in the code:
> 1. i_size_write() should be called instead of updating i_size directly.
> 2. There's a server_eof field that is generally maintained in sync
> with i_size. Need to check how that needs to be done here.
>
> I'll submit a v2 of this patch soon.
>
> --
> Regards,
> Shyam

Attached updated patch.

This was the patch that was causing test generic/074 to regress so I
took it out of for-next temporarily till we can fix it.  The version
of the patch I was using is attached


On Tue, Jan 23, 2024 at 1:48 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Tue, Jan 23, 2024 at 12:52 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > On Sun, Jan 21, 2024 at 9:03 AM <nspmangalore@gmail.com> wrote:
> > >
> > > From: Shyam Prasad N <sprasad@microsoft.com>
> > >
> > > SMB2 CLOSE command with SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB
> > > flag set is already used by the code for SMB3+.
> > > smb2_close_getattr is the function that uses this to
> > > update the inode attributes.
> > >
> > > However, we were skipping the EndOfFile info that's returned
> > > by the server. There is a small chance that the file size
> > > may have been changed in the small window between the client
> > > sending the close request (thereby giving up lease if it had)
> > > to the point that the server returns the response.
> > >
> > > This change uses the field to update the inode size.
> > > Also, it is a valid case for a zero AllocationSize to be returned
> > > by the server for the file. We were discarding such values, thereby
> > > resulting in stale i_blocks value. Fixed that here too.
> > >
> > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > ---
> > >  fs/smb/client/smb2ops.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > > index d9553c2556a2..e23577584ed6 100644
> > > --- a/fs/smb/client/smb2ops.c
> > > +++ b/fs/smb/client/smb2ops.c
> > > @@ -1433,9 +1433,9 @@ smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon,
> > >          * but instead 512 byte (2**9) size is required for
> > >          * calculating num blocks.
> > >          */
> > > -       if (le64_to_cpu(file_inf.AllocationSize) > 4096)
> > > -               inode->i_blocks =
> > > -                       (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9;
> > > +       inode->i_blocks = (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9;
> > > +
> > > +       inode->i_size = le64_to_cpu(file_inf.EndOfFile);
> > >
> > >         /* End of file and Attributes should not have to be updated on close */
> > >         spin_unlock(&inode->i_lock);
> > > --
> > > 2.34.1
> > >
> > Noticed a couple of things in other places in the code:
> > 1. i_size_write() should be called instead of updating i_size directly.
> > 2. There's a server_eof field that is generally maintained in sync
> > with i_size. Need to check how that needs to be done here.
> >
> > I'll submit a v2 of this patch soon.
> >
> > --
> > Regards,
> > Shyam
>
> Attached updated patch.
>
> --
> Regards,
> Shyam