Ronnie does make a good point about the error returned to the user. However, masking all the errors returned from the cifs_construct_tcon() doesn't sit well with me. For example, if the system is low on memory, or there's an EIO returned due to some reason, those errors will not make it to the user, and the user will see a blanket "Permission denied". How many Linux users will check the dmesg/logs when they get an EACCES? Moreover, if the actual returned is ENOMEM but we return EACCES to the user, we could have a situation where the user gets EACCES for one I/O but no such error for the subsequent I/O. This can be quite confusing for the user. Opinions? Regards, Shyam On Thu, Oct 1, 2020 at 10:55 AM Steve French <smfrench@xxxxxxxxx> wrote: > > Ronnie makes a good point about checking valid return codes by api call (open, close, read, write) as well ... > > On Thu, Oct 1, 2020 at 12:13 AM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote: >> >> Hi Ronnie, >> >> Returning EACCES to userspace does make sense. However, with the >> current code, even a ENOMEM or EIO will return a EACCES to userspace. >> I'm thinking that we need to specifically map ENOKEY to EACCES here. >> >> I was discussing this with Steve yesterday, and he suggested that I >> email fs-devel about what is the general expectation from VFS. >> >> Regards, >> Shyam >> >> On Thu, Oct 1, 2020 at 4:47 AM ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote: >> > >> > On Thu, Oct 1, 2020 at 1:57 AM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote: >> > > >> > > Ok. Checked the test. It does a multiuser mount of a file share. (I'm >> > > assuming this is using sec=ntlmssp) >> > > Then does an "ls" on the mount point as another user. Expects EACCES >> > > as the output. >> > > With this change, the command returns an ENOKEY. >> > > With the credentials added to keyring using cifscreds, the ls command works. >> > > >> > > Now the question is: Is the right error to return here EACCES or >> > > ENOKEY? Even if the expected error here should be EACCES, I'd say that >> > > the error returned by cifs_set_cifscreds should return that error. >> > > >> > >> > Good find! >> > >> > I am leaning towards leaving it as EACCES as that is a generic "you >> > dont have access" errno we report back to userspace. >> > System calls like opendir(), stat() etc all list EACCES as a valid >> > return code but they don't list ENOKEY. >> > So we then have to consider the applications. For applications that >> > don't look at errno it wouldn't matter but for applications that do >> > look at errno and try to take proper action to failures would be >> > "surprised" since they get an errno that is not listed on the manpage. >> > >> > So that is why I think we should leave it as -EACCES as this is the >> > generic "you don't have access". >> > But we should make sure that when this happens we do get a more >> > detailed "you don't have access because you don't have proper >> > credentials" into log-file / dmesg. >> > >> > Regards >> > Ronnie >> > >> > > Regards, >> > > Shyam >> > > >> > > On Wed, Sep 30, 2020 at 8:48 PM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote: >> > > > >> > > > It looks like xfstests smb3 multiuser cifsutils-101 and cifs-101 are failing. >> > > > Maybe they were written keeping in mind the current error code >> > > > returned by cifs.ko in this situation? Let me take a look. >> > > > I guess @ronnie sahlberg will be able to debug the failing tests >> > > > faster. The failing test has his name in the code. :) >> > > > >> > > > On Tue, Sep 29, 2020 at 11:16 PM Steve French <smfrench@xxxxxxxxx> wrote: >> > > > > >> > > > > tentatively merged ... running the usual functional tests >> > > > > http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/399 >> > > > > >> > > > > On Tue, Sep 29, 2020 at 8:16 AM Aurélien Aptel <aaptel@xxxxxxxx> wrote: >> > > > > > >> > > > > > Shyam Prasad N <nspmangalore@xxxxxxxxx> writes: >> > > > > > > One of the cases where this behaviour is confusing is where a >> > > > > > > new tcon needs to be constructed, but it fails due to >> > > > > > > expired keys. cifs_construct_tcon then returns ENOKEY, >> > > > > > > but we end up returning a EACCES to the user. >> > > > > > > >> > > > > > > Signed-off-by: Shyam Prasad N <sprasad@xxxxxxxxxxxxx> >> > > > > > >> > > > > > LGTM >> > > > > > >> > > > > > Reviewed-by: Aurelien Aptel <aaptel@xxxxxxxx> >> > > > > > >> > > > > > 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 >> > > > >> > > > >> > > > >> > > > -- >> > > > -Shyam >> > > >> > > >> > > >> > > -- >> > > -Shyam >> >> >> >> -- >> -Shyam > > > > -- > Thanks, > > Steve -- -Shyam
