diff mbox series

cifs: Fixed OFD locks do not conflict with eachother

Message ID CAHOrsKgdentJKif+WH3N=PNPj8fBSZbFzzkyR3_EsNOseodKcw@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series cifs: Fixed OFD locks do not conflict with eachother | expand

Commit Message

Georgy Bystrenin Dec. 19, 2018, 5:14 p.m. UTC 
While resolving a bug with locks on samba shares found a strange behavior.
When a file locked by one node and we trying to lock it from another node
it fail with errno 5 (EIO) but in that case errno must be set to
(EACCES | EAGAIN).
This isn't happening when we try to lock file second time on same node.
In this case it returns EACCES as expected.
Also this issue not reproduces when we use SMB1 protocol (vers=1.0 in
mount options).

Further investigation showed that the mapping from status_to_posix_error
is different for SMB1 and SMB2+ implementations.
For SMB1 mapping is [NT_STATUS_LOCK_NOT_GRANTED to ERRlock]
(https://github.com/torvalds/linux/blob/master/fs/cifs/netmisc.c#L66)
but for SMB2+ mapping is [STATUS_LOCK_NOT_GRANTED to -EIO]
(https://github.com/torvalds/linux/blob/master/fs/cifs/smb2maperror.c#L383)

Quick changes in SMB2+ mapping from EIO to EACCES has fixed issue.

BUG: https://bugzilla.kernel.org/show_bug.cgi?id=201971

Signed-off-by: Georgy A Bystrenin <gkot@altlinux.org>
---
 fs/cifs/smb2maperror.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

        "STATUS_CTL_FILE_NOT_SUPPORTED"},
--
2.17.1

Comments

Pavel Shilovskiy Dec. 19, 2018, 5:37 p.m. UTC | #1 
Looks good. Thanks!

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

-----Original Message-----
From: Georgy Bystrenin <gkot@basealt.ru> 
Sent: Wednesday, December 19, 2018 9:15 AM
To: linux-cifs@vger.kernel.org
Cc: Pavel Shilovskiy <pshilov@microsoft.com>; Michael Shigorin <mike@altlinux.org>; Evgeny Sinelnikov <sin@altlinux.org>
Subject: [PATCH] cifs: Fixed OFD locks do not conflict with eachother

While resolving a bug with locks on samba shares found a strange behavior.
When a file locked by one node and we trying to lock it from another node it fail with errno 5 (EIO) but in that case errno must be set to (EACCES | EAGAIN).
This isn't happening when we try to lock file second time on same node.
In this case it returns EACCES as expected.
Also this issue not reproduces when we use SMB1 protocol (vers=1.0 in mount options).

Further investigation showed that the mapping from status_to_posix_error is different for SMB1 and SMB2+ implementations.
For SMB1 mapping is [NT_STATUS_LOCK_NOT_GRANTED to ERRlock]
(https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Ffs%2Fcifs%2Fnetmisc.c%23L66&amp;data=02%7C01%7Cpshilov%40microsoft.com%7C32719cbffb6e4fa3a18208d665d588a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C636808365142084033&amp;sdata=ymTs1OSE2HBAjH%2BcYb0rmsWNxAwp%2Beg23vOE%2B6HaQkU%3D&amp;reserved=0)
but for SMB2+ mapping is [STATUS_LOCK_NOT_GRANTED to -EIO]
(https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Ffs%2Fcifs%2Fsmb2maperror.c%23L383&amp;data=02%7C01%7Cpshilov%40microsoft.com%7C32719cbffb6e4fa3a18208d665d588a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C636808365142084033&amp;sdata=hK5X5JyOJ0KGduL7Ty4cru51TOLrSaqNMY11Yeywy78%3D&amp;reserved=0)

Quick changes in SMB2+ mapping from EIO to EACCES has fixed issue.

BUG: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D201971&amp;data=02%7C01%7Cpshilov%40microsoft.com%7C32719cbffb6e4fa3a18208d665d588a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C636808365142084033&amp;sdata=D4or6WC3MuSi31KpUfIpE%2BjDC6xSEBjJCMiMNDvC3cM%3D&amp;reserved=0

Signed-off-by: Georgy A Bystrenin <gkot@altlinux.org>
---
 fs/cifs/smb2maperror.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/smb2maperror.c b/fs/cifs/smb2maperror.c index 62c88dfed57b..d7e839cb773f 100644
--- a/fs/cifs/smb2maperror.c
+++ b/fs/cifs/smb2maperror.c
@@ -378,8 +378,8 @@ static const struct status_to_posix_error smb2_error_map_table[] = {
        {STATUS_NONEXISTENT_EA_ENTRY, -EIO, "STATUS_NONEXISTENT_EA_ENTRY"},
        {STATUS_NO_EAS_ON_FILE, -ENODATA, "STATUS_NO_EAS_ON_FILE"},
        {STATUS_EA_CORRUPT_ERROR, -EIO, "STATUS_EA_CORRUPT_ERROR"},
-       {STATUS_FILE_LOCK_CONFLICT, -EIO, "STATUS_FILE_LOCK_CONFLICT"},
-       {STATUS_LOCK_NOT_GRANTED, -EIO, "STATUS_LOCK_NOT_GRANTED"},
+       {STATUS_FILE_LOCK_CONFLICT, -EACCES, "STATUS_FILE_LOCK_CONFLICT"},
+       {STATUS_LOCK_NOT_GRANTED, -EACCES, "STATUS_LOCK_NOT_GRANTED"},
        {STATUS_DELETE_PENDING, -ENOENT, "STATUS_DELETE_PENDING"},
        {STATUS_CTL_FILE_NOT_SUPPORTED, -ENOSYS,
        "STATUS_CTL_FILE_NOT_SUPPORTED"},
--
2.17.1
Steve French Dec. 21, 2018, 6:16 a.m. UTC | #2 
Patch would not apply so manually updated it and then merged to
cifs-2.6.git for-next

Please doublecheck that ok (see attached)



On Wed, Dec 19, 2018 at 11:37 AM Pavel Shilovskiy <pshilov@microsoft.com> wrote:
>
> Looks good. Thanks!
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> -----Original Message-----
> From: Georgy Bystrenin <gkot@basealt.ru>
> Sent: Wednesday, December 19, 2018 9:15 AM
> To: linux-cifs@vger.kernel.org
> Cc: Pavel Shilovskiy <pshilov@microsoft.com>; Michael Shigorin <mike@altlinux.org>; Evgeny Sinelnikov <sin@altlinux.org>
> Subject: [PATCH] cifs: Fixed OFD locks do not conflict with eachother
>
> While resolving a bug with locks on samba shares found a strange behavior.
> When a file locked by one node and we trying to lock it from another node it fail with errno 5 (EIO) but in that case errno must be set to (EACCES | EAGAIN).
> This isn't happening when we try to lock file second time on same node.
> In this case it returns EACCES as expected.
> Also this issue not reproduces when we use SMB1 protocol (vers=1.0 in mount options).
>
> Further investigation showed that the mapping from status_to_posix_error is different for SMB1 and SMB2+ implementations.
> For SMB1 mapping is [NT_STATUS_LOCK_NOT_GRANTED to ERRlock]
> (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Ffs%2Fcifs%2Fnetmisc.c%23L66&amp;data=02%7C01%7Cpshilov%40microsoft.com%7C32719cbffb6e4fa3a18208d665d588a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C636808365142084033&amp;sdata=ymTs1OSE2HBAjH%2BcYb0rmsWNxAwp%2Beg23vOE%2B6HaQkU%3D&amp;reserved=0)
> but for SMB2+ mapping is [STATUS_LOCK_NOT_GRANTED to -EIO]
> (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Ffs%2Fcifs%2Fsmb2maperror.c%23L383&amp;data=02%7C01%7Cpshilov%40microsoft.com%7C32719cbffb6e4fa3a18208d665d588a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C636808365142084033&amp;sdata=hK5X5JyOJ0KGduL7Ty4cru51TOLrSaqNMY11Yeywy78%3D&amp;reserved=0)
>
> Quick changes in SMB2+ mapping from EIO to EACCES has fixed issue.
>
> BUG: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D201971&amp;data=02%7C01%7Cpshilov%40microsoft.com%7C32719cbffb6e4fa3a18208d665d588a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C636808365142084033&amp;sdata=D4or6WC3MuSi31KpUfIpE%2BjDC6xSEBjJCMiMNDvC3cM%3D&amp;reserved=0
>
> Signed-off-by: Georgy A Bystrenin <gkot@altlinux.org>
> ---
>  fs/cifs/smb2maperror.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/smb2maperror.c b/fs/cifs/smb2maperror.c index 62c88dfed57b..d7e839cb773f 100644
> --- a/fs/cifs/smb2maperror.c
> +++ b/fs/cifs/smb2maperror.c
> @@ -378,8 +378,8 @@ static const struct status_to_posix_error smb2_error_map_table[] = {
>         {STATUS_NONEXISTENT_EA_ENTRY, -EIO, "STATUS_NONEXISTENT_EA_ENTRY"},
>         {STATUS_NO_EAS_ON_FILE, -ENODATA, "STATUS_NO_EAS_ON_FILE"},
>         {STATUS_EA_CORRUPT_ERROR, -EIO, "STATUS_EA_CORRUPT_ERROR"},
> -       {STATUS_FILE_LOCK_CONFLICT, -EIO, "STATUS_FILE_LOCK_CONFLICT"},
> -       {STATUS_LOCK_NOT_GRANTED, -EIO, "STATUS_LOCK_NOT_GRANTED"},
> +       {STATUS_FILE_LOCK_CONFLICT, -EACCES, "STATUS_FILE_LOCK_CONFLICT"},
> +       {STATUS_LOCK_NOT_GRANTED, -EACCES, "STATUS_LOCK_NOT_GRANTED"},
>         {STATUS_DELETE_PENDING, -ENOENT, "STATUS_DELETE_PENDING"},
>         {STATUS_CTL_FILE_NOT_SUPPORTED, -ENOSYS,
>         "STATUS_CTL_FILE_NOT_SUPPORTED"},
> --
> 2.17.1
Pavel Shilovsky Dec. 21, 2018, 8:03 p.m. UTC | #3 
Looks fine. I would rather change the title to somewhat more
self-explanatory like "CIFS: Fix error mapping for SMB2_LOCK command".

I think it is worth going to stable as well.

--
Best regards,
Pavel Shilovsky

пт, 21 дек. 2018 г. в 00:59, Steve French <smfrench@gmail.com>:
>
> Patch would not apply so manually updated it and then merged to
> cifs-2.6.git for-next
>
> Please doublecheck that ok (see attached)
>
>
>
> On Wed, Dec 19, 2018 at 11:37 AM Pavel Shilovskiy <pshilov@microsoft.com> wrote:
> >
> > Looks good. Thanks!
> >
> > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> >
> > -----Original Message-----
> > From: Georgy Bystrenin <gkot@basealt.ru>
> > Sent: Wednesday, December 19, 2018 9:15 AM
> > To: linux-cifs@vger.kernel.org
> > Cc: Pavel Shilovskiy <pshilov@microsoft.com>; Michael Shigorin <mike@altlinux.org>; Evgeny Sinelnikov <sin@altlinux.org>
> > Subject: [PATCH] cifs: Fixed OFD locks do not conflict with eachother
> >
> > While resolving a bug with locks on samba shares found a strange behavior.
> > When a file locked by one node and we trying to lock it from another node it fail with errno 5 (EIO) but in that case errno must be set to (EACCES | EAGAIN).
> > This isn't happening when we try to lock file second time on same node.
> > In this case it returns EACCES as expected.
> > Also this issue not reproduces when we use SMB1 protocol (vers=1.0 in mount options).
> >
> > Further investigation showed that the mapping from status_to_posix_error is different for SMB1 and SMB2+ implementations.
> > For SMB1 mapping is [NT_STATUS_LOCK_NOT_GRANTED to ERRlock]
> > (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Ffs%2Fcifs%2Fnetmisc.c%23L66&amp;data=02%7C01%7Cpshilov%40microsoft.com%7C32719cbffb6e4fa3a18208d665d588a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C636808365142084033&amp;sdata=ymTs1OSE2HBAjH%2BcYb0rmsWNxAwp%2Beg23vOE%2B6HaQkU%3D&amp;reserved=0)
> > but for SMB2+ mapping is [STATUS_LOCK_NOT_GRANTED to -EIO]
> > (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Ffs%2Fcifs%2Fsmb2maperror.c%23L383&amp;data=02%7C01%7Cpshilov%40microsoft.com%7C32719cbffb6e4fa3a18208d665d588a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C636808365142084033&amp;sdata=hK5X5JyOJ0KGduL7Ty4cru51TOLrSaqNMY11Yeywy78%3D&amp;reserved=0)
> >
> > Quick changes in SMB2+ mapping from EIO to EACCES has fixed issue.
> >
> > BUG: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D201971&amp;data=02%7C01%7Cpshilov%40microsoft.com%7C32719cbffb6e4fa3a18208d665d588a8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C1%7C636808365142084033&amp;sdata=D4or6WC3MuSi31KpUfIpE%2BjDC6xSEBjJCMiMNDvC3cM%3D&amp;reserved=0
> >
> > Signed-off-by: Georgy A Bystrenin <gkot@altlinux.org>
> > ---
> >  fs/cifs/smb2maperror.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/cifs/smb2maperror.c b/fs/cifs/smb2maperror.c index 62c88dfed57b..d7e839cb773f 100644
> > --- a/fs/cifs/smb2maperror.c
> > +++ b/fs/cifs/smb2maperror.c
> > @@ -378,8 +378,8 @@ static const struct status_to_posix_error smb2_error_map_table[] = {
> >         {STATUS_NONEXISTENT_EA_ENTRY, -EIO, "STATUS_NONEXISTENT_EA_ENTRY"},
> >         {STATUS_NO_EAS_ON_FILE, -ENODATA, "STATUS_NO_EAS_ON_FILE"},
> >         {STATUS_EA_CORRUPT_ERROR, -EIO, "STATUS_EA_CORRUPT_ERROR"},
> > -       {STATUS_FILE_LOCK_CONFLICT, -EIO, "STATUS_FILE_LOCK_CONFLICT"},
> > -       {STATUS_LOCK_NOT_GRANTED, -EIO, "STATUS_LOCK_NOT_GRANTED"},
> > +       {STATUS_FILE_LOCK_CONFLICT, -EACCES, "STATUS_FILE_LOCK_CONFLICT"},
> > +       {STATUS_LOCK_NOT_GRANTED, -EACCES, "STATUS_LOCK_NOT_GRANTED"},
> >         {STATUS_DELETE_PENDING, -ENOENT, "STATUS_DELETE_PENDING"},
> >         {STATUS_CTL_FILE_NOT_SUPPORTED, -ENOSYS,
> >         "STATUS_CTL_FILE_NOT_SUPPORTED"},
> > --
> > 2.17.1
>
>
>
> --
> Thanks,
>
> Steve
diff mbox series

Patch

diff --git a/fs/cifs/smb2maperror.c b/fs/cifs/smb2maperror.c
index 62c88dfed57b..d7e839cb773f 100644
--- a/fs/cifs/smb2maperror.c
+++ b/fs/cifs/smb2maperror.c
@@ -378,8 +378,8 @@  static const struct status_to_posix_error
smb2_error_map_table[] = {
        {STATUS_NONEXISTENT_EA_ENTRY, -EIO, "STATUS_NONEXISTENT_EA_ENTRY"},
        {STATUS_NO_EAS_ON_FILE, -ENODATA, "STATUS_NO_EAS_ON_FILE"},
        {STATUS_EA_CORRUPT_ERROR, -EIO, "STATUS_EA_CORRUPT_ERROR"},
-       {STATUS_FILE_LOCK_CONFLICT, -EIO, "STATUS_FILE_LOCK_CONFLICT"},
-       {STATUS_LOCK_NOT_GRANTED, -EIO, "STATUS_LOCK_NOT_GRANTED"},
+       {STATUS_FILE_LOCK_CONFLICT, -EACCES, "STATUS_FILE_LOCK_CONFLICT"},
+       {STATUS_LOCK_NOT_GRANTED, -EACCES, "STATUS_LOCK_NOT_GRANTED"},
        {STATUS_DELETE_PENDING, -ENOENT, "STATUS_DELETE_PENDING"},
        {STATUS_CTL_FILE_NOT_SUPPORTED, -ENOSYS,