[PATCH] cifs: Return the appropriate error in cifs_sb_tlink instead of a generic error.

* [PATCH] cifs: Return the appropriate error in cifs_sb_tlink instead of a generic error.
@ 2020-09-29 12:02 Shyam Prasad N
  2020-09-29 13:16 ` Aurélien Aptel
  0 siblings, 1 reply; 9+ messages in thread
From: Shyam Prasad N @ 2020-09-29 12:02 UTC (permalink / raw)
  To: Steve French, CIFS, sribhat.msa, rohiths.msft

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

Hi Steve,

Please find the attached patch for fixing the issue of returning hard
coded EACCES when a new tcon needs to be established. This bug affects
only the multiuser scenario.

I've tested out this fix for a few scenarios:
1. User already has valid krb5 tickets, and mount is attempted.
2. User doesn't have krb5 tickets, and mount is attempted.
3. User has expired krb5 tickets, and mount is attempted.
4. The share is already mounted, and the mount point is accessed as
another user who does not have valid krb5 tickets.
5. Same as 4, but created another session as same user (to validate
the case of existing tcon).

Please review the changes.

-- 
-Shyam

[-- Attachment #2: 0001-cifs-Return-the-appropriate-error-in-cifs_sb_tlink.patch --]
[-- Type: application/octet-stream, Size: 1657 bytes --]

From f634b932e480f76743fc9c4f1cab5500eefff898 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Tue, 29 Sep 2020 00:06:43 -0700
Subject: [PATCH] cifs: Return the appropriate error in cifs_sb_tlink instead
 of a generic error.

Currently, cifs_sb_tlink returns a fixed errno EACCES when it
fails for most reasons. This ends up masking the error returned
by cifs_construct_tcon, which will have a more meaningful error
for the failure.

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@microsoft.com>
---
 fs/cifs/connect.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 1a3b7793095e..5fda76f41404 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -5475,8 +5475,9 @@ cifs_sb_tlink(struct cifs_sb_info *cifs_sb)
 
 		/* return error if we tried this already recently */
 		if (time_before(jiffies, tlink->tl_time + TLINK_ERROR_EXPIRE)) {
+			newtlink = (struct tcon_link *) tlink->tl_tcon;
 			cifs_put_tlink(tlink);
-			return ERR_PTR(-EACCES);
+			return newtlink;
 		}
 
 		if (test_and_set_bit(TCON_LINK_PENDING, &tlink->tl_flags))
@@ -5488,8 +5489,9 @@ cifs_sb_tlink(struct cifs_sb_info *cifs_sb)
 	wake_up_bit(&tlink->tl_flags, TCON_LINK_PENDING);
 
 	if (IS_ERR(tlink->tl_tcon)) {
+		newtlink = (struct tcon_link *) tlink->tl_tcon;
 		cifs_put_tlink(tlink);
-		return ERR_PTR(-EACCES);
+		return newtlink;
 	}
 
 	return tlink;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread