* [PATCH 0/1] don't leak EDEADLK to userspace @ 2021-08-25 11:16 Ronnie Sahlberg 2021-08-25 11:16 ` [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED Ronnie Sahlberg 0 siblings, 1 reply; 6+ messages in thread From: Ronnie Sahlberg @ 2021-08-25 11:16 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French Steve, Version 2: refactor the patch so we catch all (both) places where we call initiate_cifs_search() and update the commit message ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED 2021-08-25 11:16 [PATCH 0/1] don't leak EDEADLK to userspace Ronnie Sahlberg @ 2021-08-25 11:16 ` Ronnie Sahlberg 2021-08-25 21:09 ` Steve French 0 siblings, 1 reply; 6+ messages in thread From: Ronnie Sahlberg @ 2021-08-25 11:16 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French RHBZ: 1994393 If we hit a STATUS_USER_SESSION_DELETED for the Create part in the Create/QueryDirectory compound that starts a directory scan we will leak EDEADLK back to userspace and surprise glibc and the application. Pick this up initiate_cifs_search() and retry a small number of tries before we return an error to userspace. Cc: stable@vger.kernel.org Reported-by: Xiaoli Feng <xifeng@redhat.com> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/readdir.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c index bfee176b901d..4518e3ca64df 100644 --- a/fs/cifs/readdir.c +++ b/fs/cifs/readdir.c @@ -369,7 +369,7 @@ int get_symlink_reparse_path(char *full_path, struct cifs_sb_info *cifs_sb, */ static int -initiate_cifs_search(const unsigned int xid, struct file *file, +_initiate_cifs_search(const unsigned int xid, struct file *file, const char *full_path) { __u16 search_flags; @@ -451,6 +451,23 @@ initiate_cifs_search(const unsigned int xid, struct file *file, return rc; } +static int +initiate_cifs_search(const unsigned int xid, struct file *file, + const char *full_path) +{ + int rc, retry_count = 0; + + do { + rc = _initiate_cifs_search(xid, file, full_path); + /* + * We don't have enough credits to start reading the + * directory so just try again. + */ + } while (rc == -EDEADLK && retry_count++ < 5); + + return rc; +} + /* return length of unicode string in bytes */ static int cifs_unicode_bytelen(const char *str) { -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED 2021-08-25 11:16 ` [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED Ronnie Sahlberg @ 2021-08-25 21:09 ` Steve French 2021-08-26 17:15 ` Shyam Prasad N 0 siblings, 1 reply; 6+ messages in thread From: Steve French @ 2021-08-25 21:09 UTC (permalink / raw) To: Ronnie Sahlberg; +Cc: linux-cifs [-- Attachment #1: Type: text/plain, Size: 1965 bytes --] lightly updated to add short sleep before retry On Wed, Aug 25, 2021 at 6:17 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > RHBZ: 1994393 > > If we hit a STATUS_USER_SESSION_DELETED for the Create part in the > Create/QueryDirectory compound that starts a directory scan > we will leak EDEADLK back to userspace and surprise glibc and the application. > > Pick this up initiate_cifs_search() and retry a small number of tries before we > return an error to userspace. > > Cc: stable@vger.kernel.org > Reported-by: Xiaoli Feng <xifeng@redhat.com> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/readdir.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > index bfee176b901d..4518e3ca64df 100644 > --- a/fs/cifs/readdir.c > +++ b/fs/cifs/readdir.c > @@ -369,7 +369,7 @@ int get_symlink_reparse_path(char *full_path, struct cifs_sb_info *cifs_sb, > */ > > static int > -initiate_cifs_search(const unsigned int xid, struct file *file, > +_initiate_cifs_search(const unsigned int xid, struct file *file, > const char *full_path) > { > __u16 search_flags; > @@ -451,6 +451,23 @@ initiate_cifs_search(const unsigned int xid, struct file *file, > return rc; > } > > +static int > +initiate_cifs_search(const unsigned int xid, struct file *file, > + const char *full_path) > +{ > + int rc, retry_count = 0; > + > + do { > + rc = _initiate_cifs_search(xid, file, full_path); > + /* > + * We don't have enough credits to start reading the > + * directory so just try again. > + */ > + } while (rc == -EDEADLK && retry_count++ < 5); > + > + return rc; > +} > + > /* return length of unicode string in bytes */ > static int cifs_unicode_bytelen(const char *str) > { > -- > 2.30.2 > -- Thanks, Steve [-- Attachment #2: 0001-cifs-Do-not-leak-EDEADLK-to-dgetents64-for-STATUS_US.patch --] [-- Type: text/x-patch, Size: 1950 bytes --] From 57cea50fa5a30068752a8155e1c7230c8c585493 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg <lsahlber@redhat.com> Date: Wed, 25 Aug 2021 21:16:56 +1000 Subject: [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED RHBZ: 1994393 If we hit a STATUS_USER_SESSION_DELETED for the Create part in the Create/QueryDirectory compound that starts a directory scan we will leak EDEADLK back to userspace and surprise glibc and the application. Pick this up initiate_cifs_search() and retry a small number of tries before we return an error to userspace. Cc: stable@vger.kernel.org Reported-by: Xiaoli Feng <xifeng@redhat.com> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/readdir.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c index bfee176b901d..54d77c99e21c 100644 --- a/fs/cifs/readdir.c +++ b/fs/cifs/readdir.c @@ -369,7 +369,7 @@ int get_symlink_reparse_path(char *full_path, struct cifs_sb_info *cifs_sb, */ static int -initiate_cifs_search(const unsigned int xid, struct file *file, +_initiate_cifs_search(const unsigned int xid, struct file *file, const char *full_path) { __u16 search_flags; @@ -451,6 +451,27 @@ initiate_cifs_search(const unsigned int xid, struct file *file, return rc; } +static int +initiate_cifs_search(const unsigned int xid, struct file *file, + const char *full_path) +{ + int rc, retry_count = 0; + + do { + rc = _initiate_cifs_search(xid, file, full_path); + /* + * If we don't have enough credits to start reading the + * directory just try again after short wait. + */ + if (rc != -EDEADLK) + break; + + usleep_range(512, 2048); + } while (retry_count++ < 5); + + return rc; +} + /* return length of unicode string in bytes */ static int cifs_unicode_bytelen(const char *str) { -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED 2021-08-25 21:09 ` Steve French @ 2021-08-26 17:15 ` Shyam Prasad N 2021-08-26 18:07 ` ronnie sahlberg 0 siblings, 1 reply; 6+ messages in thread From: Shyam Prasad N @ 2021-08-26 17:15 UTC (permalink / raw) To: Steve French; +Cc: Ronnie Sahlberg, linux-cifs On Thu, Aug 26, 2021 at 2:39 AM Steve French <smfrench@gmail.com> wrote: > > lightly updated to add short sleep before retry > > > On Wed, Aug 25, 2021 at 6:17 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > > > RHBZ: 1994393 > > > > If we hit a STATUS_USER_SESSION_DELETED for the Create part in the > > Create/QueryDirectory compound that starts a directory scan > > we will leak EDEADLK back to userspace and surprise glibc and the application. > > > > Pick this up initiate_cifs_search() and retry a small number of tries before we > > return an error to userspace. > > > > Cc: stable@vger.kernel.org > > Reported-by: Xiaoli Feng <xifeng@redhat.com> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/readdir.c | 19 ++++++++++++++++++- > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > > index bfee176b901d..4518e3ca64df 100644 > > --- a/fs/cifs/readdir.c > > +++ b/fs/cifs/readdir.c > > @@ -369,7 +369,7 @@ int get_symlink_reparse_path(char *full_path, struct cifs_sb_info *cifs_sb, > > */ > > > > static int > > -initiate_cifs_search(const unsigned int xid, struct file *file, > > +_initiate_cifs_search(const unsigned int xid, struct file *file, > > const char *full_path) > > { > > __u16 search_flags; > > @@ -451,6 +451,23 @@ initiate_cifs_search(const unsigned int xid, struct file *file, > > return rc; > > } > > > > +static int > > +initiate_cifs_search(const unsigned int xid, struct file *file, > > + const char *full_path) > > +{ > > + int rc, retry_count = 0; > > + > > + do { > > + rc = _initiate_cifs_search(xid, file, full_path); > > + /* > > + * We don't have enough credits to start reading the > > + * directory so just try again. > > + */ > > + } while (rc == -EDEADLK && retry_count++ < 5); > > + > > + return rc; > > +} > > + > > /* return length of unicode string in bytes */ > > static int cifs_unicode_bytelen(const char *str) > > { > > -- > > 2.30.2 > > > > > -- > Thanks, > > Steve Hi Ronnie, EDEADLK is returned in wait_for_compound_request, when num of credits is 0, but there are no in flight requests to get more credits from. Why did we reach here in the first place? If we already found STATUS_USER_SESSION_DELETED, why are we waiting for another request? -- Regards, Shyam ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED 2021-08-26 17:15 ` Shyam Prasad N @ 2021-08-26 18:07 ` ronnie sahlberg 0 siblings, 0 replies; 6+ messages in thread From: ronnie sahlberg @ 2021-08-26 18:07 UTC (permalink / raw) To: Shyam Prasad N; +Cc: Steve French, Ronnie Sahlberg, linux-cifs On Fri, Aug 27, 2021 at 3:16 AM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Thu, Aug 26, 2021 at 2:39 AM Steve French <smfrench@gmail.com> wrote: > > > > lightly updated to add short sleep before retry > > > > > > On Wed, Aug 25, 2021 at 6:17 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > > > > > RHBZ: 1994393 > > > > > > If we hit a STATUS_USER_SESSION_DELETED for the Create part in the > > > Create/QueryDirectory compound that starts a directory scan > > > we will leak EDEADLK back to userspace and surprise glibc and the application. > > > > > > Pick this up initiate_cifs_search() and retry a small number of tries before we > > > return an error to userspace. > > > > > > Cc: stable@vger.kernel.org > > > Reported-by: Xiaoli Feng <xifeng@redhat.com> > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > --- > > > fs/cifs/readdir.c | 19 ++++++++++++++++++- > > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > > > index bfee176b901d..4518e3ca64df 100644 > > > --- a/fs/cifs/readdir.c > > > +++ b/fs/cifs/readdir.c > > > @@ -369,7 +369,7 @@ int get_symlink_reparse_path(char *full_path, struct cifs_sb_info *cifs_sb, > > > */ > > > > > > static int > > > -initiate_cifs_search(const unsigned int xid, struct file *file, > > > +_initiate_cifs_search(const unsigned int xid, struct file *file, > > > const char *full_path) > > > { > > > __u16 search_flags; > > > @@ -451,6 +451,23 @@ initiate_cifs_search(const unsigned int xid, struct file *file, > > > return rc; > > > } > > > > > > +static int > > > +initiate_cifs_search(const unsigned int xid, struct file *file, > > > + const char *full_path) > > > +{ > > > + int rc, retry_count = 0; > > > + > > > + do { > > > + rc = _initiate_cifs_search(xid, file, full_path); > > > + /* > > > + * We don't have enough credits to start reading the > > > + * directory so just try again. > > > + */ > > > + } while (rc == -EDEADLK && retry_count++ < 5); > > > + > > > + return rc; > > > +} > > > + > > > /* return length of unicode string in bytes */ > > > static int cifs_unicode_bytelen(const char *str) > > > { > > > -- > > > 2.30.2 > > > > > > > > > -- > > Thanks, > > > > Steve > > Hi Ronnie, > > EDEADLK is returned in wait_for_compound_request, when num of credits > is 0, but there are no in flight requests to get more credits from. > Why did we reach here in the first place? If we already found > STATUS_USER_SESSION_DELETED, why are we waiting for another request? USER_SESSION_DELETED means the session is bad and needs to be reconnected which is why we can not get any credits. We can't get any credits until later until later been reconnected. If this happens from smb2_query_dir_first, the first attempt with USER_SESSION_DELETED will cause the session to need a reconnect and return -EAGAIN. While we have a retry on -EAGAIN here in this function, I don;t it can handle cases where we have both EAGAIN but also a situation where the session is dead and needs reconnect (which also means no credits). I think we are too deep in the call-stack and with too many things locked that we can not reconnect the session right now. Thus the retry on EAGAIN turns into a EDEADLK. The EDEADLK is then returned through the stack all the way back to cifs_readdir() where we don't have all these things locked and thus a re-try will actually trigger a reconnect and we recover. It is easy to reproduce with scrambla and the small error inject patch I posted. Every third "ls /mountpoint" will return a USERS_SESSION_DELETED to smb2_query_dir_first which you can then see leaking -EDEADLK to the ls command if you strace it. BTW. I really want to start using scrambla in our buildbot since it will allow us to do a lot of error injection from server side and test that we can recover correctly from them in the client. (scrambla is ~5k lines of python3 and is a lot less intimidating to patch to inject errors than full blown samba) > > -- > Regards, > Shyam ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 0/1] cifs: do not leak EDEADLK to dgetents64 @ 2021-08-25 7:30 Ronnie Sahlberg 2021-08-25 7:30 ` [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED Ronnie Sahlberg 0 siblings, 1 reply; 6+ messages in thread From: Ronnie Sahlberg @ 2021-08-25 7:30 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French Steve, List Please find a patch that stops us from leaking EDEADLK (not enough credits) to userspace when we do not have enough credits due to a pending reconnect. This can be triggered for example if the server responds with STATUS_USER_SESSION_DELETED during the Create part of the Create/QueryDir that starts a directory scan. Easiest way to reproduce this is patching up scrambla to inject this error every 3 directory scans: diff --git a/server/server.py b/server/server.py index 7fd113b..47d0b7f 100644 --- a/server/server.py +++ b/server/server.py @@ -26,6 +26,7 @@ from smb2.filesystem_info import * from smb2.dir_info import * from smb2.ntlmssp import * + class File(object): def __init__(self, path, flags, at, **kwargs): @@ -81,6 +82,7 @@ class Server(object): dialect = 0 def __init__(self, s, **kwargs): + self.errc = 0 self._s = s self._sesid = 1 self._treeid = 1 @@ -348,6 +350,16 @@ class Server(object): # # Create/Open # + #print('PDU', pdu) + if pdu['desired_access'] == 0x81: + print('YEAH') + self.errc = self.errc + 1 + if self.errc == 3: + print('Generate error') + self.errc = 0 + self._compound_error = Status.INVALID_PARAMETER + return (Status.USER_SESSION_DELETED, + ErrorResponse.encode({'error_data' : bytes(1)})) if not hdr['tree_id'] in self.trees: self._compound_error = Status.INVALID_PARAMETER return (self._compound_error, ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED 2021-08-25 7:30 [PATCH 0/1] cifs: do not leak EDEADLK to dgetents64 Ronnie Sahlberg @ 2021-08-25 7:30 ` Ronnie Sahlberg 0 siblings, 0 replies; 6+ messages in thread From: Ronnie Sahlberg @ 2021-08-25 7:30 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French RHBZ: 1994393 If we hit a STATUS_USER_SESSION_DELETED for the Create part in the Create/QueryDirectory compound that starts a directory scan we will leak EDEADLK back to userspace and surprise glibc and the application. Pick this up cifs_readdir() and retry a small number of tries before we return an error to userspace. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/readdir.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c index bfee176b901d..56e5d456366d 100644 --- a/fs/cifs/readdir.c +++ b/fs/cifs/readdir.c @@ -930,6 +930,7 @@ int cifs_readdir(struct file *file, struct dir_context *ctx) unsigned int max_len; const char *full_path; void *page = alloc_dentry_path(); + int retry_count = 0; xid = get_xid(); @@ -944,8 +945,15 @@ int cifs_readdir(struct file *file, struct dir_context *ctx) * '..'. Otherwise we won't be able to notify VFS in case of failure. */ if (file->private_data == NULL) { + again: rc = initiate_cifs_search(xid, file, full_path); - cifs_dbg(FYI, "initiate cifs search rc %d\n", rc); + if (rc == -EDEADLK && retry_count++ < 5) { + /* + * We don't have enough credits to start reading the + * directory so just try again. + */ + goto again; + } if (rc) goto rddir2_exit; } -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-26 18:07 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-25 11:16 [PATCH 0/1] don't leak EDEADLK to userspace Ronnie Sahlberg 2021-08-25 11:16 ` [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED Ronnie Sahlberg 2021-08-25 21:09 ` Steve French 2021-08-26 17:15 ` Shyam Prasad N 2021-08-26 18:07 ` ronnie sahlberg -- strict thread matches above, loose matches on Subject: below -- 2021-08-25 7:30 [PATCH 0/1] cifs: do not leak EDEADLK to dgetents64 Ronnie Sahlberg 2021-08-25 7:30 ` [PATCH] cifs: Do not leak EDEADLK to dgetents64 for STATUS_USER_SESSION_DELETED Ronnie Sahlberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).