[PATCH] cifs: handle -EINTR in cifs_setattr

[PATCH] cifs: handle -EINTR in cifs_setattr

[Ronnie Sahlberg]

RHBZ: 1848178

Some calls that set attributes, like utimensat(), are not supposed to return
-EINTR and thus do not have handlers for this in glibc which causes us
to leak -EINTR to the applications which are also unprepared to handle it.

For example tar will break if utimensat() return -EINTR and abort unpacking
the archive. Other applications may break too.

To handle this we add checks, and retry, for -EINTR in cifs_setattr()

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/inode.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3989d08396ac..2dd6e7902ff4 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2879,13 +2879,18 @@ cifs_setattr(struct dentry *direntry, struct iattr *attrs)
 {
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
 	struct cifs_tcon *pTcon = cifs_sb_master_tcon(cifs_sb);
+	int rc, retries = 0;
 
-	if (pTcon->unix_ext)
-		return cifs_setattr_unix(direntry, attrs);
-
-	return cifs_setattr_nounix(direntry, attrs);
+	do {
+		if (pTcon->unix_ext)
+			rc = cifs_setattr_unix(direntry, attrs);
+		else
+			rc = cifs_setattr_nounix(direntry, attrs);
+		retries++;
+	} while (rc == -EINTR && retries < 4);
 
 	/* BB: add cifs_setattr_legacy for really old servers */
+	return rc;
 }
 
 #if 0
-- 
2.13.6

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[Aurélien Aptel]

Hi Ronnie,

Ronnie Sahlberg <lsahlber@redhat.com> writes:
> Some calls that set attributes, like utimensat(), are not supposed to return
> -EINTR and thus do not have handlers for this in glibc which causes us
> to leak -EINTR to the applications which are also unprepared to handle it.

EINTR happens when the task receives a signal right?

https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html

Given what you said and what the glibc doc reads it seems like the fix
should go in glibc. Otherwise we need to care about every single syscall.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[ronnie sahlberg]

On Tue, Oct 6, 2020 at 8:57 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Hi Ronnie,
>
> Ronnie Sahlberg <lsahlber@redhat.com> writes:
> > Some calls that set attributes, like utimensat(), are not supposed to return
> > -EINTR and thus do not have handlers for this in glibc which causes us
> > to leak -EINTR to the applications which are also unprepared to handle it.
>
> EINTR happens when the task receives a signal right?
>
> https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html
>
> Given what you said and what the glibc doc reads it seems like the fix
> should go in glibc. Otherwise we need to care about every single syscall.

glibc have handling for EINTR in most places, but not in for example utimensat()
because this function is not supposed to be able to return this error.
Similarly we have functions like chmod and chown that also come into cifs.ko
via the same entrypoint: cifs_setattr()
I think all of these "update inode data" are never supposed to be
interruptible since
they were classically just updating the in-memory inode and the thread
would never hit d-state.

Anyway, for these functions EINTR is not a valid return code so I
think we should take care to not
return it. Even if we change glibc adn the very very thin wrapper for
this functions there are applications
that might call the systemcall directly or via a different c-library.


>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[Steve French]

Tentatively merged updated version of this patch into cifs-2.6.git
for-next pending more testing and reviews

On Tue, Oct 6, 2020 at 5:22 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote:
>
> On Tue, Oct 6, 2020 at 8:57 PM Aurélien Aptel <aaptel@suse.com> wrote:
> >
> > Hi Ronnie,
> >
> > Ronnie Sahlberg <lsahlber@redhat.com> writes:
> > > Some calls that set attributes, like utimensat(), are not supposed to return
> > > -EINTR and thus do not have handlers for this in glibc which causes us
> > > to leak -EINTR to the applications which are also unprepared to handle it.
> >
> > EINTR happens when the task receives a signal right?
> >
> > https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html
> >
> > Given what you said and what the glibc doc reads it seems like the fix
> > should go in glibc. Otherwise we need to care about every single syscall.
>
> glibc have handling for EINTR in most places, but not in for example utimensat()
> because this function is not supposed to be able to return this error.
> Similarly we have functions like chmod and chown that also come into cifs.ko
> via the same entrypoint: cifs_setattr()
> I think all of these "update inode data" are never supposed to be
> interruptible since
> they were classically just updating the in-memory inode and the thread
> would never hit d-state.
>
> Anyway, for these functions EINTR is not a valid return code so I
> think we should take care to not
> return it. Even if we change glibc adn the very very thin wrapper for
> this functions there are applications
> that might call the systemcall directly or via a different c-library.
>
>
> >
> > Cheers,
> > --
> > Aurélien Aptel / SUSE Labs Samba Team
> > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)



-- 
Thanks,

Steve

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[Steve French]

Also FYI- I made a minor modification after discussing with Ronnie,
reducing the retry count

On Tue, Oct 6, 2020 at 8:44 PM Steve French <smfrench@gmail.com> wrote:
>
> Tentatively merged updated version of this patch into cifs-2.6.git
> for-next pending more testing and reviews
>
> On Tue, Oct 6, 2020 at 5:22 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote:
> >
> > On Tue, Oct 6, 2020 at 8:57 PM Aurélien Aptel <aaptel@suse.com> wrote:
> > >
> > > Hi Ronnie,
> > >
> > > Ronnie Sahlberg <lsahlber@redhat.com> writes:
> > > > Some calls that set attributes, like utimensat(), are not supposed to return
> > > > -EINTR and thus do not have handlers for this in glibc which causes us
> > > > to leak -EINTR to the applications which are also unprepared to handle it.
> > >
> > > EINTR happens when the task receives a signal right?
> > >
> > > https://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html
> > >
> > > Given what you said and what the glibc doc reads it seems like the fix
> > > should go in glibc. Otherwise we need to care about every single syscall.
> >
> > glibc have handling for EINTR in most places, but not in for example utimensat()
> > because this function is not supposed to be able to return this error.
> > Similarly we have functions like chmod and chown that also come into cifs.ko
> > via the same entrypoint: cifs_setattr()
> > I think all of these "update inode data" are never supposed to be
> > interruptible since
> > they were classically just updating the in-memory inode and the thread
> > would never hit d-state.
> >
> > Anyway, for these functions EINTR is not a valid return code so I
> > think we should take care to not
> > return it. Even if we change glibc adn the very very thin wrapper for
> > this functions there are applications
> > that might call the systemcall directly or via a different c-library.
> >
> >
> > >
> > > Cheers,
> > > --
> > > Aurélien Aptel / SUSE Labs Samba Team
> > > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[Aurélien Aptel]

ronnie sahlberg <ronniesahlberg@gmail.com> writes:
> glibc have handling for EINTR in most places, but not in for example utimensat()
> because this function is not supposed to be able to return this error.
> Similarly we have functions like chmod and chown that also come into cifs.ko
> via the same entrypoint: cifs_setattr()
> I think all of these "update inode data" are never supposed to be
> interruptible since
> they were classically just updating the in-memory inode and the thread
> would never hit d-state.

I see, thanks for the explanation.

> Anyway, for these functions EINTR is not a valid return code so I
> think we should take care to not
> return it. Even if we change glibc adn the very very thin wrapper for
> this functions there are applications
> that might call the systemcall directly or via a different c-library.

Should we use is_retryable_error() to check for the errcode?

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[ronnie sahlberg]

On Wed, Oct 7, 2020 at 8:45 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> ronnie sahlberg <ronniesahlberg@gmail.com> writes:
> > glibc have handling for EINTR in most places, but not in for example utimensat()
> > because this function is not supposed to be able to return this error.
> > Similarly we have functions like chmod and chown that also come into cifs.ko
> > via the same entrypoint: cifs_setattr()
> > I think all of these "update inode data" are never supposed to be
> > interruptible since
> > they were classically just updating the in-memory inode and the thread
> > would never hit d-state.
>
> I see, thanks for the explanation.
>
> > Anyway, for these functions EINTR is not a valid return code so I
> > think we should take care to not
> > return it. Even if we change glibc adn the very very thin wrapper for
> > this functions there are applications
> > that might call the systemcall directly or via a different c-library.
>
> Should we use is_retryable_error() to check for the errcode?


That is probably a good idea.
I will resend the patch with that suggestion.

Thanks!
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[Steve French]

merged updated patch into cifs-2.6.git for-next

On Thu, Oct 8, 2020 at 6:33 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> RHBZ: 1848178
>
> Some calls that set attributes, like utimensat(), are not supposed to return
> -EINTR and thus do not have handlers for this in glibc which causes us
> to leak -EINTR to the applications which are also unprepared to handle it.
>
> For example tar will break if utimensat() return -EINTR and abort unpacking
> the archive. Other applications may break too.
>
> To handle this we add checks, and retry, for -EINTR in cifs_setattr()
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/inode.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 3989d08396ac..0bd22c41a623 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -2879,13 +2879,18 @@ cifs_setattr(struct dentry *direntry, struct iattr *attrs)
>  {
>         struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
>         struct cifs_tcon *pTcon = cifs_sb_master_tcon(cifs_sb);
> +       int rc, retries = 0;
>
> -       if (pTcon->unix_ext)
> -               return cifs_setattr_unix(direntry, attrs);
> -
> -       return cifs_setattr_nounix(direntry, attrs);
> +       do {
> +               if (pTcon->unix_ext)
> +                       rc = cifs_setattr_unix(direntry, attrs);
> +               else
> +                       rc = cifs_setattr_nounix(direntry, attrs);
> +               retries++;
> +       } while (is_retryable_error(rc) && retries < 2);
>
>         /* BB: add cifs_setattr_legacy for really old servers */
> +       return rc;
>  }
>
>  #if 0
> --
> 2.13.6
>


-- 
Thanks,

Steve

[PATCH] cifs: handle -EINTR in cifs_setattr

[Ronnie Sahlberg]

RHBZ: 1848178

Some calls that set attributes, like utimensat(), are not supposed to return
-EINTR and thus do not have handlers for this in glibc which causes us
to leak -EINTR to the applications which are also unprepared to handle it.

For example tar will break if utimensat() return -EINTR and abort unpacking
the archive. Other applications may break too.

To handle this we add checks, and retry, for -EINTR in cifs_setattr()

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/inode.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3989d08396ac..0bd22c41a623 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2879,13 +2879,18 @@ cifs_setattr(struct dentry *direntry, struct iattr *attrs)
 {
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
 	struct cifs_tcon *pTcon = cifs_sb_master_tcon(cifs_sb);
+	int rc, retries = 0;
 
-	if (pTcon->unix_ext)
-		return cifs_setattr_unix(direntry, attrs);
-
-	return cifs_setattr_nounix(direntry, attrs);
+	do {
+		if (pTcon->unix_ext)
+			rc = cifs_setattr_unix(direntry, attrs);
+		else
+			rc = cifs_setattr_nounix(direntry, attrs);
+		retries++;
+	} while (is_retryable_error(rc) && retries < 2);
 
 	/* BB: add cifs_setattr_legacy for really old servers */
+	return rc;
 }
 
 #if 0
-- 
2.13.6

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[ronnie sahlberg]

On Tue, Oct 6, 2020 at 3:06 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> Question: What causes cifs_setattr_* to return -EINTR?
> Does an infinite reattempt make sense here? Or should we give up after
> N attempts?

Good question. Probably.
I think you are right we might need to limit the retries.

One  case could be where we never receive a response from the server
and userspace
wants to CTRL-C the thread.  It might mean we have to do N CTRL-C to
end the process but N is better than infinite CTRL-C :-)

I will resend with a limit on retries.

Thanks!

> Also, should we cifsFYI log before we re-attempt?
>
> Regards,
> Shyam
>
> On Tue, Oct 6, 2020 at 10:03 AM Steve French <smfrench@gmail.com> wrote:
> >
> > tentatively merged into cifs-2.6.git for-next
> >
> > On Mon, Oct 5, 2020 at 9:26 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> > >
> > > RHBZ: 1848178
> > >
> > > Some calls that set attributes, like utimensat(), are not supposed to return
> > > -EINTR and thus do not have handlers for this in glibc which causes us
> > > to leak -EINTR to the applications which are also unprepared to handle it.
> > >
> > > For example tar will break if utimensat() return -EINTR and abort unpacking
> > > the archive. Other applications may break too.
> > >
> > > To handle this we add checks, and retry, for -EINTR in cifs_setattr()
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > >  fs/cifs/inode.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > > index 3989d08396ac..74ed12f2c8aa 100644
> > > --- a/fs/cifs/inode.c
> > > +++ b/fs/cifs/inode.c
> > > @@ -2879,13 +2879,17 @@ cifs_setattr(struct dentry *direntry, struct iattr *attrs)
> > >  {
> > >         struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
> > >         struct cifs_tcon *pTcon = cifs_sb_master_tcon(cifs_sb);
> > > +       int rc;
> > >
> > > -       if (pTcon->unix_ext)
> > > -               return cifs_setattr_unix(direntry, attrs);
> > > -
> > > -       return cifs_setattr_nounix(direntry, attrs);
> > > +       do {
> > > +               if (pTcon->unix_ext)
> > > +                       rc = cifs_setattr_unix(direntry, attrs);
> > > +               else
> > > +                       rc = cifs_setattr_nounix(direntry, attrs);
> > > +       } while (rc == -EINTR);
> > >
> > >         /* BB: add cifs_setattr_legacy for really old servers */
> > > +       return rc;
> > >  }
> > >
> > >  #if 0
> > > --
> > > 2.13.6
> > >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> -Shyam

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[Shyam Prasad N]

Ignore my second question. I got the answer in your description.

On Tue, Oct 6, 2020 at 10:36 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> Question: What causes cifs_setattr_* to return -EINTR?
> Does an infinite reattempt make sense here? Or should we give up after
> N attempts?
> Also, should we cifsFYI log before we re-attempt?
>
> Regards,
> Shyam
>
> On Tue, Oct 6, 2020 at 10:03 AM Steve French <smfrench@gmail.com> wrote:
> >
> > tentatively merged into cifs-2.6.git for-next
> >
> > On Mon, Oct 5, 2020 at 9:26 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> > >
> > > RHBZ: 1848178
> > >
> > > Some calls that set attributes, like utimensat(), are not supposed to return
> > > -EINTR and thus do not have handlers for this in glibc which causes us
> > > to leak -EINTR to the applications which are also unprepared to handle it.
> > >
> > > For example tar will break if utimensat() return -EINTR and abort unpacking
> > > the archive. Other applications may break too.
> > >
> > > To handle this we add checks, and retry, for -EINTR in cifs_setattr()
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > >  fs/cifs/inode.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > > index 3989d08396ac..74ed12f2c8aa 100644
> > > --- a/fs/cifs/inode.c
> > > +++ b/fs/cifs/inode.c
> > > @@ -2879,13 +2879,17 @@ cifs_setattr(struct dentry *direntry, struct iattr *attrs)
> > >  {
> > >         struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
> > >         struct cifs_tcon *pTcon = cifs_sb_master_tcon(cifs_sb);
> > > +       int rc;
> > >
> > > -       if (pTcon->unix_ext)
> > > -               return cifs_setattr_unix(direntry, attrs);
> > > -
> > > -       return cifs_setattr_nounix(direntry, attrs);
> > > +       do {
> > > +               if (pTcon->unix_ext)
> > > +                       rc = cifs_setattr_unix(direntry, attrs);
> > > +               else
> > > +                       rc = cifs_setattr_nounix(direntry, attrs);
> > > +       } while (rc == -EINTR);
> > >
> > >         /* BB: add cifs_setattr_legacy for really old servers */
> > > +       return rc;
> > >  }
> > >
> > >  #if 0
> > > --
> > > 2.13.6
> > >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> -Shyam



-- 
-Shyam

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[Shyam Prasad N]

Question: What causes cifs_setattr_* to return -EINTR?
Does an infinite reattempt make sense here? Or should we give up after
N attempts?
Also, should we cifsFYI log before we re-attempt?

Regards,
Shyam

On Tue, Oct 6, 2020 at 10:03 AM Steve French <smfrench@gmail.com> wrote:
>
> tentatively merged into cifs-2.6.git for-next
>
> On Mon, Oct 5, 2020 at 9:26 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> >
> > RHBZ: 1848178
> >
> > Some calls that set attributes, like utimensat(), are not supposed to return
> > -EINTR and thus do not have handlers for this in glibc which causes us
> > to leak -EINTR to the applications which are also unprepared to handle it.
> >
> > For example tar will break if utimensat() return -EINTR and abort unpacking
> > the archive. Other applications may break too.
> >
> > To handle this we add checks, and retry, for -EINTR in cifs_setattr()
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/inode.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > index 3989d08396ac..74ed12f2c8aa 100644
> > --- a/fs/cifs/inode.c
> > +++ b/fs/cifs/inode.c
> > @@ -2879,13 +2879,17 @@ cifs_setattr(struct dentry *direntry, struct iattr *attrs)
> >  {
> >         struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
> >         struct cifs_tcon *pTcon = cifs_sb_master_tcon(cifs_sb);
> > +       int rc;
> >
> > -       if (pTcon->unix_ext)
> > -               return cifs_setattr_unix(direntry, attrs);
> > -
> > -       return cifs_setattr_nounix(direntry, attrs);
> > +       do {
> > +               if (pTcon->unix_ext)
> > +                       rc = cifs_setattr_unix(direntry, attrs);
> > +               else
> > +                       rc = cifs_setattr_nounix(direntry, attrs);
> > +       } while (rc == -EINTR);
> >
> >         /* BB: add cifs_setattr_legacy for really old servers */
> > +       return rc;
> >  }
> >
> >  #if 0
> > --
> > 2.13.6
> >
>
>
> --
> Thanks,
>
> Steve



-- 
-Shyam

Re: [PATCH] cifs: handle -EINTR in cifs_setattr

[Steve French]

tentatively merged into cifs-2.6.git for-next

On Mon, Oct 5, 2020 at 9:26 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> RHBZ: 1848178
>
> Some calls that set attributes, like utimensat(), are not supposed to return
> -EINTR and thus do not have handlers for this in glibc which causes us
> to leak -EINTR to the applications which are also unprepared to handle it.
>
> For example tar will break if utimensat() return -EINTR and abort unpacking
> the archive. Other applications may break too.
>
> To handle this we add checks, and retry, for -EINTR in cifs_setattr()
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/inode.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 3989d08396ac..74ed12f2c8aa 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -2879,13 +2879,17 @@ cifs_setattr(struct dentry *direntry, struct iattr *attrs)
>  {
>         struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
>         struct cifs_tcon *pTcon = cifs_sb_master_tcon(cifs_sb);
> +       int rc;
>
> -       if (pTcon->unix_ext)
> -               return cifs_setattr_unix(direntry, attrs);
> -
> -       return cifs_setattr_nounix(direntry, attrs);
> +       do {
> +               if (pTcon->unix_ext)
> +                       rc = cifs_setattr_unix(direntry, attrs);
> +               else
> +                       rc = cifs_setattr_nounix(direntry, attrs);
> +       } while (rc == -EINTR);
>
>         /* BB: add cifs_setattr_legacy for really old servers */
> +       return rc;
>  }
>
>  #if 0
> --
> 2.13.6
>


-- 
Thanks,

Steve

[PATCH] cifs: handle -EINTR in cifs_setattr

[Ronnie Sahlberg]

RHBZ: 1848178

Some calls that set attributes, like utimensat(), are not supposed to return
-EINTR and thus do not have handlers for this in glibc which causes us
to leak -EINTR to the applications which are also unprepared to handle it.

For example tar will break if utimensat() return -EINTR and abort unpacking
the archive. Other applications may break too.

To handle this we add checks, and retry, for -EINTR in cifs_setattr()

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/inode.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3989d08396ac..74ed12f2c8aa 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2879,13 +2879,17 @@ cifs_setattr(struct dentry *direntry, struct iattr *attrs)
 {
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);
 	struct cifs_tcon *pTcon = cifs_sb_master_tcon(cifs_sb);
+	int rc;
 
-	if (pTcon->unix_ext)
-		return cifs_setattr_unix(direntry, attrs);
-
-	return cifs_setattr_nounix(direntry, attrs);
+	do {
+		if (pTcon->unix_ext)
+			rc = cifs_setattr_unix(direntry, attrs);
+		else
+			rc = cifs_setattr_nounix(direntry, attrs);
+	} while (rc == -EINTR);
 
 	/* BB: add cifs_setattr_legacy for really old servers */
+	return rc;
 }
 
 #if 0
-- 
2.13.6