| Message ID | 20201006052643.6298-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: handle -EINTR in cifs_setattr | expand |
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,
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)
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)
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
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,
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)
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
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(-)