On Thu, Oct 1, 2020 at 8:42 PM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote: > > 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. It is primarily about making sure that applications do not misbehave. Especially applications or glibc that do pay close attention to what errno is when a syscall fails. And where errno drives them how to handle or manage recovery. It basically depends on case by case scenario. Are applications calling this syscall directly or via glibc. If the latter, what errnos does glibc accept for that syscall and how does it deal with them? The answer here is what the man page says, because that is what application and glibc are coded against. We can not introduce new errno values to userspace. There may be situations where currently we leak "unexpected" errno values through syscalls. That would be a bug and we need to fix it. In cifs.ko we have had very serious bugs in the past where we were leaking "unexpected" errno values to either glibc or application that turned what should be a simple recoverable error into a serious hard Data-Loss failure :-(. By causing glibc/application recovery code to be "surprised" and to fail recovery. Very sad. The way imho opinion to prevent confusion to end user is to log warning with explanation to dmesg or log file. If user caser, user looks at dmesg/log. If user does not care, that is fine. As long as we do not "surprise" glibc or application. We make sure we return only those errnos in the manpage and from there it is the responsibility of glibc/application to handle it. > > 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
