* [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one (try #2)
@ 2010-11-16 7:48 Pavel Shilovsky
[not found] ` <1289893710-7352-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 16+ messages in thread
From: Pavel Shilovsky @ 2010-11-16 7:48 UTC (permalink / raw)
To: linux-cifs-u79uwXL29TY76Z2rM5mHXA
Make connect logic more ip-protocol independent. Also move RFC1001 stuff into
separate function.
Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
fs/cifs/connect.c | 314 ++++++++++++++++++++---------------------------------
1 files changed, 116 insertions(+), 198 deletions(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 251a17c..8bdf1cc 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -114,8 +114,8 @@ struct smb_vol {
#define TLINK_ERROR_EXPIRE (1 * HZ)
#define TLINK_IDLE_EXPIRE (600 * HZ)
-static int ipv4_connect(struct TCP_Server_Info *server);
-static int ipv6_connect(struct TCP_Server_Info *server);
+static int ip_connect(struct TCP_Server_Info *server);
+static int generic_ip_connect(struct TCP_Server_Info *server);
static void tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink);
static void cifs_prune_tlinks(struct work_struct *work);
@@ -199,10 +199,10 @@ cifs_reconnect(struct TCP_Server_Info *server)
while ((server->tcpStatus != CifsExiting) &&
(server->tcpStatus != CifsGood)) {
try_to_freeze();
- if (server->addr.sockAddr6.sin6_family == AF_INET6)
- rc = ipv6_connect(server);
- else
- rc = ipv4_connect(server);
+
+ /* on reconnect we should try only the port we connected
+ before */
+ rc = generic_ip_connect(server);
if (rc) {
cFYI(1, "reconnect error %d", rc);
msleep(3000);
@@ -1668,12 +1668,11 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
/* other OS never observed in Wild doing 139 with v6 */
memcpy(&tcp_ses->addr.sockAddr6, sin_server6,
sizeof(struct sockaddr_in6));
- rc = ipv6_connect(tcp_ses);
- } else {
+ } else
memcpy(&tcp_ses->addr.sockAddr, sin_server,
sizeof(struct sockaddr_in));
- rc = ipv4_connect(tcp_ses);
- }
+
+ rc = ip_connect(tcp_ses);
if (rc < 0) {
cERROR(1, "Error connecting to socket. Aborting operation");
goto out_err_crypto_release;
@@ -2121,19 +2120,97 @@ bind_socket(struct TCP_Server_Info *server)
}
static int
-ipv4_connect(struct TCP_Server_Info *server)
+ip_rfc1001_connect(struct TCP_Server_Info *server)
+{
+ int rc = 0;
+ /* some servers require RFC1001 sessinit before sending
+ negprot - BB check reconnection in case where second
+ sessinit is sent but no second negprot */
+ struct rfc1002_session_packet *ses_init_buf;
+ struct smb_hdr *smb_buf;
+ ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
+ GFP_KERNEL);
+ if (ses_init_buf) {
+ ses_init_buf->trailer.session_req.called_len = 32;
+
+ if (server->server_RFC1001_name &&
+ server->server_RFC1001_name[0] != 0)
+ rfc1002mangle(ses_init_buf->trailer.
+ session_req.called_name,
+ server->server_RFC1001_name,
+ RFC1001_NAME_LEN_WITH_NULL);
+ else
+ rfc1002mangle(ses_init_buf->trailer.
+ session_req.called_name,
+ DEFAULT_CIFS_CALLED_NAME,
+ RFC1001_NAME_LEN_WITH_NULL);
+
+ ses_init_buf->trailer.session_req.calling_len = 32;
+
+ /* calling name ends in null (byte 16) from old smb
+ convention. */
+ if (server->workstation_RFC1001_name &&
+ server->workstation_RFC1001_name[0] != 0)
+ rfc1002mangle(ses_init_buf->trailer.
+ session_req.calling_name,
+ server->workstation_RFC1001_name,
+ RFC1001_NAME_LEN_WITH_NULL);
+ else
+ rfc1002mangle(ses_init_buf->trailer.
+ session_req.calling_name,
+ "LINUX_CIFS_CLNT",
+ RFC1001_NAME_LEN_WITH_NULL);
+
+ ses_init_buf->trailer.session_req.scope1 = 0;
+ ses_init_buf->trailer.session_req.scope2 = 0;
+ smb_buf = (struct smb_hdr *)ses_init_buf;
+
+ /* sizeof RFC1002_SESSION_REQUEST with no scope */
+ smb_buf->smb_buf_length = 0x81000044;
+ rc = smb_send(server, smb_buf, 0x44);
+ kfree(ses_init_buf);
+ msleep(1); /* RFC1001 layer in at least one server
+ requires very short break before negprot
+ presumably because not expecting negprot
+ to follow so fast. This is a simple
+ solution that works without
+ complicating the code and causes no
+ significant slowing down on mount
+ for everyone else */
+ }
+ /* else the negprot may still work without this
+ even though malloc failed */
+
+ return rc;
+}
+
+static int
+generic_ip_connect(struct TCP_Server_Info *server)
{
int rc = 0;
- int val;
- bool connected = false;
- __be16 orig_port = 0;
+ bool using_ipv6 = false;
+ unsigned short int *sport;
+ int slen;
struct socket *socket = server->ssocket;
+ struct sockaddr *saddr;
+
+ if (server->addr.sockAddr6.sin6_family == AF_INET6) {
+ sport = &server->addr.sockAddr6.sin6_port;
+ using_ipv6 = true;
+ slen = sizeof(struct sockaddr_in6);
+ saddr = (struct sockaddr *) &server->addr.sockAddr6;
+ } else {
+ sport = &server->addr.sockAddr.sin_port;
+ slen = sizeof(struct sockaddr_in);
+ saddr = (struct sockaddr *) &server->addr.sockAddr;
+ }
if (socket == NULL) {
rc = sock_create_kern(PF_INET, SOCK_STREAM,
IPPROTO_TCP, &socket);
if (rc < 0) {
cERROR(1, "Error %d creating socket", rc);
+ server->ssocket = NULL;
return rc;
}
@@ -2141,59 +2218,24 @@ ipv4_connect(struct TCP_Server_Info *server)
cFYI(1, "Socket created");
server->ssocket = socket;
socket->sk->sk_allocation = GFP_NOFS;
- cifs_reclassify_socket4(socket);
+ if (using_ipv6)
+ cifs_reclassify_socket6(socket);
+ else
+ cifs_reclassify_socket4(socket);
}
rc = bind_socket(server);
if (rc < 0)
return rc;
- /* user overrode default port */
- if (server->addr.sockAddr.sin_port) {
- rc = socket->ops->connect(socket, (struct sockaddr *)
- &server->addr.sockAddr,
- sizeof(struct sockaddr_in), 0);
- if (rc >= 0)
- connected = true;
- }
-
- if (!connected) {
- /* save original port so we can retry user specified port
- later if fall back ports fail this time */
- orig_port = server->addr.sockAddr.sin_port;
-
- /* do not retry on the same port we just failed on */
- if (server->addr.sockAddr.sin_port != htons(CIFS_PORT)) {
- server->addr.sockAddr.sin_port = htons(CIFS_PORT);
- rc = socket->ops->connect(socket,
- (struct sockaddr *)
- &server->addr.sockAddr,
- sizeof(struct sockaddr_in), 0);
- if (rc >= 0)
- connected = true;
- }
- }
- if (!connected) {
- server->addr.sockAddr.sin_port = htons(RFC1001_PORT);
- rc = socket->ops->connect(socket, (struct sockaddr *)
- &server->addr.sockAddr,
- sizeof(struct sockaddr_in), 0);
- if (rc >= 0)
- connected = true;
- }
-
- /* give up here - unless we want to retry on different
- protocol families some day */
- if (!connected) {
- if (orig_port)
- server->addr.sockAddr.sin_port = orig_port;
- cFYI(1, "Error %d connecting to server via ipv4", rc);
+ rc = socket->ops->connect(socket, saddr, slen, 0);
+ if (rc < 0) {
+ cFYI(1, "Error %d connecting to server", rc);
sock_release(socket);
server->ssocket = NULL;
return rc;
}
-
/*
* Eventually check for other socket options to change from
* the default. sock_setsockopt not used because it expects
@@ -2211,7 +2253,7 @@ ipv4_connect(struct TCP_Server_Info *server)
}
if (server->tcp_nodelay) {
- val = 1;
+ int val = 1;
rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
(char *)&val, sizeof(val));
if (rc)
@@ -2222,161 +2264,37 @@ ipv4_connect(struct TCP_Server_Info *server)
socket->sk->sk_sndbuf,
socket->sk->sk_rcvbuf, socket->sk->sk_rcvtimeo);
- /* send RFC1001 sessinit */
- if (server->addr.sockAddr.sin_port == htons(RFC1001_PORT)) {
- /* some servers require RFC1001 sessinit before sending
- negprot - BB check reconnection in case where second
- sessinit is sent but no second negprot */
- struct rfc1002_session_packet *ses_init_buf;
- struct smb_hdr *smb_buf;
- ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
- GFP_KERNEL);
- if (ses_init_buf) {
- ses_init_buf->trailer.session_req.called_len = 32;
- if (server->server_RFC1001_name &&
- server->server_RFC1001_name[0] != 0)
- rfc1002mangle(ses_init_buf->trailer.
- session_req.called_name,
- server->server_RFC1001_name,
- RFC1001_NAME_LEN_WITH_NULL);
- else
- rfc1002mangle(ses_init_buf->trailer.
- session_req.called_name,
- DEFAULT_CIFS_CALLED_NAME,
- RFC1001_NAME_LEN_WITH_NULL);
-
- ses_init_buf->trailer.session_req.calling_len = 32;
-
- /* calling name ends in null (byte 16) from old smb
- convention. */
- if (server->workstation_RFC1001_name &&
- server->workstation_RFC1001_name[0] != 0)
- rfc1002mangle(ses_init_buf->trailer.
- session_req.calling_name,
- server->workstation_RFC1001_name,
- RFC1001_NAME_LEN_WITH_NULL);
- else
- rfc1002mangle(ses_init_buf->trailer.
- session_req.calling_name,
- "LINUX_CIFS_CLNT",
- RFC1001_NAME_LEN_WITH_NULL);
-
- ses_init_buf->trailer.session_req.scope1 = 0;
- ses_init_buf->trailer.session_req.scope2 = 0;
- smb_buf = (struct smb_hdr *)ses_init_buf;
- /* sizeof RFC1002_SESSION_REQUEST with no scope */
- smb_buf->smb_buf_length = 0x81000044;
- rc = smb_send(server, smb_buf, 0x44);
- kfree(ses_init_buf);
- msleep(1); /* RFC1001 layer in at least one server
- requires very short break before negprot
- presumably because not expecting negprot
- to follow so fast. This is a simple
- solution that works without
- complicating the code and causes no
- significant slowing down on mount
- for everyone else */
- }
- /* else the negprot may still work without this
- even though malloc failed */
-
- }
+ if (*sport == htons(RFC1001_PORT))
+ rc = ip_rfc1001_connect(server);
return rc;
}
static int
-ipv6_connect(struct TCP_Server_Info *server)
+ip_connect(struct TCP_Server_Info *server)
{
- int rc = 0;
- int val;
- bool connected = false;
- __be16 orig_port = 0;
- struct socket *socket = server->ssocket;
+ unsigned short int *sport;
- if (socket == NULL) {
- rc = sock_create_kern(PF_INET6, SOCK_STREAM,
- IPPROTO_TCP, &socket);
- if (rc < 0) {
- cERROR(1, "Error %d creating ipv6 socket", rc);
- socket = NULL;
- return rc;
- }
+ if (server->addr.sockAddr6.sin6_family == AF_INET6)
+ sport = &server->addr.sockAddr6.sin6_port;
+ else
+ sport = &server->addr.sockAddr.sin_port;
- /* BB other socket options to set KEEPALIVE, NODELAY? */
- cFYI(1, "ipv6 Socket created");
- server->ssocket = socket;
- socket->sk->sk_allocation = GFP_NOFS;
- cifs_reclassify_socket6(socket);
- }
+ if (*sport == 0) {
+ int rc;
- rc = bind_socket(server);
- if (rc < 0)
- return rc;
+ /* try with 445 port at first */
+ *sport = htons(CIFS_PORT);
- /* user overrode default port */
- if (server->addr.sockAddr6.sin6_port) {
- rc = socket->ops->connect(socket,
- (struct sockaddr *) &server->addr.sockAddr6,
- sizeof(struct sockaddr_in6), 0);
+ rc = generic_ip_connect(server);
if (rc >= 0)
- connected = true;
- }
-
- if (!connected) {
- /* save original port so we can retry user specified port
- later if fall back ports fail this time */
-
- orig_port = server->addr.sockAddr6.sin6_port;
- /* do not retry on the same port we just failed on */
- if (server->addr.sockAddr6.sin6_port != htons(CIFS_PORT)) {
- server->addr.sockAddr6.sin6_port = htons(CIFS_PORT);
- rc = socket->ops->connect(socket, (struct sockaddr *)
- &server->addr.sockAddr6,
- sizeof(struct sockaddr_in6), 0);
- if (rc >= 0)
- connected = true;
- }
- }
- if (!connected) {
- server->addr.sockAddr6.sin6_port = htons(RFC1001_PORT);
- rc = socket->ops->connect(socket, (struct sockaddr *)
- &server->addr.sockAddr6,
- sizeof(struct sockaddr_in6), 0);
- if (rc >= 0)
- connected = true;
- }
-
- /* give up here - unless we want to retry on different
- protocol families some day */
- if (!connected) {
- if (orig_port)
- server->addr.sockAddr6.sin6_port = orig_port;
- cFYI(1, "Error %d connecting to server via ipv6", rc);
- sock_release(socket);
- server->ssocket = NULL;
- return rc;
- }
-
- /*
- * Eventually check for other socket options to change from
- * the default. sock_setsockopt not used because it expects
- * user space buffer
- */
- socket->sk->sk_rcvtimeo = 7 * HZ;
- socket->sk->sk_sndtimeo = 5 * HZ;
+ return rc;
- if (server->tcp_nodelay) {
- val = 1;
- rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
- (char *)&val, sizeof(val));
- if (rc)
- cFYI(1, "set TCP_NODELAY socket option error %d", rc);
+ /* if it failed, try with 139 port */
+ *sport = htons(RFC1001_PORT);
}
- server->ssocket = socket;
-
- return rc;
+ return generic_ip_connect(server);
}
void reset_cifs_unix_caps(int xid, struct cifsTconInfo *tcon,
--
1.7.3.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection (try #2)
[not found] ` <1289893710-7352-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2010-11-16 7:48 ` Pavel Shilovsky
[not found] ` <1289893710-7352-2-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-11-16 18:17 ` [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one " Jeff Layton
2010-11-22 17:01 ` Jeff Layton
2 siblings, 1 reply; 16+ messages in thread
From: Pavel Shilovsky @ 2010-11-16 7:48 UTC (permalink / raw)
To: linux-cifs-u79uwXL29TY76Z2rM5mHXA
If we have a share mounted by non-standard port and try to mount another share
on the same host with standard port, we connect to the first share again -
that's wrong. This patch fixes this bug.
Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
fs/cifs/connect.c | 42 ++++++++++++++++++++++++++++++++++++------
1 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 8bdf1cc..7956e7c 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1439,6 +1439,39 @@ srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
}
}
+/* If no port specified in addr structure, we try to match with 445 port
+ and if it fails - with 139 ports */
+static bool
+match_port(struct TCP_Server_Info *server, struct sockaddr *addr)
+{
+ struct sockaddr_in *addr4 = (struct sockaddr_in *)addr;
+ struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)addr;
+ unsigned short int port, *sport;
+
+ switch (addr->sa_family) {
+ case AF_INET:
+ sport = &server->addr.sockAddr.sin_port;
+ port = addr4->sin_port;
+ break;
+ case AF_INET6:
+ sport = &server->addr.sockAddr6.sin6_port;
+ port = addr6->sin6_port;
+ break;
+ default:
+ WARN_ON(1);
+ return false;
+ }
+
+ if (!port) {
+ port = htons(CIFS_PORT);
+ if (port == *sport)
+ return true;
+
+ port = htons(RFC1001_PORT);
+ }
+
+ return port == *sport;
+}
static bool
match_address(struct TCP_Server_Info *server, struct sockaddr *addr,
@@ -1452,9 +1485,6 @@ match_address(struct TCP_Server_Info *server, struct sockaddr *addr,
if (addr4->sin_addr.s_addr !=
server->addr.sockAddr.sin_addr.s_addr)
return false;
- if (addr4->sin_port &&
- addr4->sin_port != server->addr.sockAddr.sin_port)
- return false;
break;
case AF_INET6:
if (!ipv6_addr_equal(&addr6->sin6_addr,
@@ -1463,9 +1493,6 @@ match_address(struct TCP_Server_Info *server, struct sockaddr *addr,
if (addr6->sin6_scope_id !=
server->addr.sockAddr6.sin6_scope_id)
return false;
- if (addr6->sin6_port &&
- addr6->sin6_port != server->addr.sockAddr6.sin6_port)
- return false;
break;
}
@@ -1534,6 +1561,9 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
(struct sockaddr *)&vol->srcaddr))
continue;
+ if (!match_port(server, addr))
+ continue;
+
if (!match_security(server, vol))
continue;
--
1.7.3.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one (try #2)
[not found] ` <1289893710-7352-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-11-16 7:48 ` [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection " Pavel Shilovsky
@ 2010-11-16 18:17 ` Jeff Layton
[not found] ` <20101116131755.37043f53-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2010-11-22 17:01 ` Jeff Layton
2 siblings, 1 reply; 16+ messages in thread
From: Jeff Layton @ 2010-11-16 18:17 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Tue, 16 Nov 2010 10:48:29 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Make connect logic more ip-protocol independent. Also move RFC1001 stuff into
> separate function.
>
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> fs/cifs/connect.c | 314 ++++++++++++++++++++---------------------------------
> 1 files changed, 116 insertions(+), 198 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 251a17c..8bdf1cc 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -114,8 +114,8 @@ struct smb_vol {
> #define TLINK_ERROR_EXPIRE (1 * HZ)
> #define TLINK_IDLE_EXPIRE (600 * HZ)
>
> -static int ipv4_connect(struct TCP_Server_Info *server);
> -static int ipv6_connect(struct TCP_Server_Info *server);
> +static int ip_connect(struct TCP_Server_Info *server);
> +static int generic_ip_connect(struct TCP_Server_Info *server);
> static void tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink);
> static void cifs_prune_tlinks(struct work_struct *work);
>
> @@ -199,10 +199,10 @@ cifs_reconnect(struct TCP_Server_Info *server)
> while ((server->tcpStatus != CifsExiting) &&
> (server->tcpStatus != CifsGood)) {
> try_to_freeze();
> - if (server->addr.sockAddr6.sin6_family == AF_INET6)
> - rc = ipv6_connect(server);
> - else
> - rc = ipv4_connect(server);
> +
> + /* on reconnect we should try only the port we connected
> + before */
> + rc = generic_ip_connect(server);
> if (rc) {
> cFYI(1, "reconnect error %d", rc);
> msleep(3000);
> @@ -1668,12 +1668,11 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
> /* other OS never observed in Wild doing 139 with v6 */
> memcpy(&tcp_ses->addr.sockAddr6, sin_server6,
> sizeof(struct sockaddr_in6));
> - rc = ipv6_connect(tcp_ses);
> - } else {
> + } else
> memcpy(&tcp_ses->addr.sockAddr, sin_server,
> sizeof(struct sockaddr_in));
> - rc = ipv4_connect(tcp_ses);
> - }
> +
> + rc = ip_connect(tcp_ses);
> if (rc < 0) {
> cERROR(1, "Error connecting to socket. Aborting operation");
> goto out_err_crypto_release;
> @@ -2121,19 +2120,97 @@ bind_socket(struct TCP_Server_Info *server)
> }
>
> static int
> -ipv4_connect(struct TCP_Server_Info *server)
> +ip_rfc1001_connect(struct TCP_Server_Info *server)
> +{
> + int rc = 0;
> + /* some servers require RFC1001 sessinit before sending
> + negprot - BB check reconnection in case where second
> + sessinit is sent but no second negprot */
> + struct rfc1002_session_packet *ses_init_buf;
> + struct smb_hdr *smb_buf;
> + ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
> + GFP_KERNEL);
> + if (ses_init_buf) {
> + ses_init_buf->trailer.session_req.called_len = 32;
> +
> + if (server->server_RFC1001_name &&
> + server->server_RFC1001_name[0] != 0)
> + rfc1002mangle(ses_init_buf->trailer.
> + session_req.called_name,
> + server->server_RFC1001_name,
> + RFC1001_NAME_LEN_WITH_NULL);
> + else
> + rfc1002mangle(ses_init_buf->trailer.
> + session_req.called_name,
> + DEFAULT_CIFS_CALLED_NAME,
> + RFC1001_NAME_LEN_WITH_NULL);
> +
> + ses_init_buf->trailer.session_req.calling_len = 32;
> +
> + /* calling name ends in null (byte 16) from old smb
> + convention. */
> + if (server->workstation_RFC1001_name &&
> + server->workstation_RFC1001_name[0] != 0)
> + rfc1002mangle(ses_init_buf->trailer.
> + session_req.calling_name,
> + server->workstation_RFC1001_name,
> + RFC1001_NAME_LEN_WITH_NULL);
> + else
> + rfc1002mangle(ses_init_buf->trailer.
> + session_req.calling_name,
> + "LINUX_CIFS_CLNT",
> + RFC1001_NAME_LEN_WITH_NULL);
> +
> + ses_init_buf->trailer.session_req.scope1 = 0;
> + ses_init_buf->trailer.session_req.scope2 = 0;
> + smb_buf = (struct smb_hdr *)ses_init_buf;
> +
> + /* sizeof RFC1002_SESSION_REQUEST with no scope */
> + smb_buf->smb_buf_length = 0x81000044;
> + rc = smb_send(server, smb_buf, 0x44);
> + kfree(ses_init_buf);
> + msleep(1); /* RFC1001 layer in at least one server
> + requires very short break before negprot
> + presumably because not expecting negprot
> + to follow so fast. This is a simple
> + solution that works without
> + complicating the code and causes no
> + significant slowing down on mount
> + for everyone else */
> + }
> + /* else the negprot may still work without this
> + even though malloc failed */
> +
> + return rc;
> +}
> +
> +static int
> +generic_ip_connect(struct TCP_Server_Info *server)
> {
> int rc = 0;
> - int val;
> - bool connected = false;
> - __be16 orig_port = 0;
> + bool using_ipv6 = false;
> + unsigned short int *sport;
> + int slen;
> struct socket *socket = server->ssocket;
> + struct sockaddr *saddr;
> +
> + if (server->addr.sockAddr6.sin6_family == AF_INET6) {
> + sport = &server->addr.sockAddr6.sin6_port;
> + using_ipv6 = true;
> + slen = sizeof(struct sockaddr_in6);
> + saddr = (struct sockaddr *) &server->addr.sockAddr6;
> + } else {
> + sport = &server->addr.sockAddr.sin_port;
> + slen = sizeof(struct sockaddr_in);
> + saddr = (struct sockaddr *) &server->addr.sockAddr;
> + }
>
> if (socket == NULL) {
> rc = sock_create_kern(PF_INET, SOCK_STREAM,
> IPPROTO_TCP, &socket);
> if (rc < 0) {
> cERROR(1, "Error %d creating socket", rc);
> + server->ssocket = NULL;
> return rc;
> }
>
> @@ -2141,59 +2218,24 @@ ipv4_connect(struct TCP_Server_Info *server)
> cFYI(1, "Socket created");
> server->ssocket = socket;
> socket->sk->sk_allocation = GFP_NOFS;
> - cifs_reclassify_socket4(socket);
> + if (using_ipv6)
> + cifs_reclassify_socket6(socket);
> + else
> + cifs_reclassify_socket4(socket);
> }
>
> rc = bind_socket(server);
> if (rc < 0)
> return rc;
>
> - /* user overrode default port */
> - if (server->addr.sockAddr.sin_port) {
> - rc = socket->ops->connect(socket, (struct sockaddr *)
> - &server->addr.sockAddr,
> - sizeof(struct sockaddr_in), 0);
> - if (rc >= 0)
> - connected = true;
> - }
> -
> - if (!connected) {
> - /* save original port so we can retry user specified port
> - later if fall back ports fail this time */
> - orig_port = server->addr.sockAddr.sin_port;
> -
> - /* do not retry on the same port we just failed on */
> - if (server->addr.sockAddr.sin_port != htons(CIFS_PORT)) {
> - server->addr.sockAddr.sin_port = htons(CIFS_PORT);
> - rc = socket->ops->connect(socket,
> - (struct sockaddr *)
> - &server->addr.sockAddr,
> - sizeof(struct sockaddr_in), 0);
> - if (rc >= 0)
> - connected = true;
> - }
> - }
> - if (!connected) {
> - server->addr.sockAddr.sin_port = htons(RFC1001_PORT);
> - rc = socket->ops->connect(socket, (struct sockaddr *)
> - &server->addr.sockAddr,
> - sizeof(struct sockaddr_in), 0);
> - if (rc >= 0)
> - connected = true;
> - }
> -
> - /* give up here - unless we want to retry on different
> - protocol families some day */
> - if (!connected) {
> - if (orig_port)
> - server->addr.sockAddr.sin_port = orig_port;
> - cFYI(1, "Error %d connecting to server via ipv4", rc);
> + rc = socket->ops->connect(socket, saddr, slen, 0);
> + if (rc < 0) {
> + cFYI(1, "Error %d connecting to server", rc);
> sock_release(socket);
> server->ssocket = NULL;
> return rc;
> }
>
> -
> /*
> * Eventually check for other socket options to change from
> * the default. sock_setsockopt not used because it expects
> @@ -2211,7 +2253,7 @@ ipv4_connect(struct TCP_Server_Info *server)
> }
>
> if (server->tcp_nodelay) {
> - val = 1;
> + int val = 1;
> rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
> (char *)&val, sizeof(val));
> if (rc)
> @@ -2222,161 +2264,37 @@ ipv4_connect(struct TCP_Server_Info *server)
> socket->sk->sk_sndbuf,
> socket->sk->sk_rcvbuf, socket->sk->sk_rcvtimeo);
>
> - /* send RFC1001 sessinit */
> - if (server->addr.sockAddr.sin_port == htons(RFC1001_PORT)) {
> - /* some servers require RFC1001 sessinit before sending
> - negprot - BB check reconnection in case where second
> - sessinit is sent but no second negprot */
> - struct rfc1002_session_packet *ses_init_buf;
> - struct smb_hdr *smb_buf;
> - ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
> - GFP_KERNEL);
> - if (ses_init_buf) {
> - ses_init_buf->trailer.session_req.called_len = 32;
> - if (server->server_RFC1001_name &&
> - server->server_RFC1001_name[0] != 0)
> - rfc1002mangle(ses_init_buf->trailer.
> - session_req.called_name,
> - server->server_RFC1001_name,
> - RFC1001_NAME_LEN_WITH_NULL);
> - else
> - rfc1002mangle(ses_init_buf->trailer.
> - session_req.called_name,
> - DEFAULT_CIFS_CALLED_NAME,
> - RFC1001_NAME_LEN_WITH_NULL);
> -
> - ses_init_buf->trailer.session_req.calling_len = 32;
> -
> - /* calling name ends in null (byte 16) from old smb
> - convention. */
> - if (server->workstation_RFC1001_name &&
> - server->workstation_RFC1001_name[0] != 0)
> - rfc1002mangle(ses_init_buf->trailer.
> - session_req.calling_name,
> - server->workstation_RFC1001_name,
> - RFC1001_NAME_LEN_WITH_NULL);
> - else
> - rfc1002mangle(ses_init_buf->trailer.
> - session_req.calling_name,
> - "LINUX_CIFS_CLNT",
> - RFC1001_NAME_LEN_WITH_NULL);
> -
> - ses_init_buf->trailer.session_req.scope1 = 0;
> - ses_init_buf->trailer.session_req.scope2 = 0;
> - smb_buf = (struct smb_hdr *)ses_init_buf;
> - /* sizeof RFC1002_SESSION_REQUEST with no scope */
> - smb_buf->smb_buf_length = 0x81000044;
> - rc = smb_send(server, smb_buf, 0x44);
> - kfree(ses_init_buf);
> - msleep(1); /* RFC1001 layer in at least one server
> - requires very short break before negprot
> - presumably because not expecting negprot
> - to follow so fast. This is a simple
> - solution that works without
> - complicating the code and causes no
> - significant slowing down on mount
> - for everyone else */
> - }
> - /* else the negprot may still work without this
> - even though malloc failed */
> -
> - }
> + if (*sport == htons(RFC1001_PORT))
> + rc = ip_rfc1001_connect(server);
>
> return rc;
> }
>
> static int
> -ipv6_connect(struct TCP_Server_Info *server)
> +ip_connect(struct TCP_Server_Info *server)
> {
> - int rc = 0;
> - int val;
> - bool connected = false;
> - __be16 orig_port = 0;
> - struct socket *socket = server->ssocket;
> + unsigned short int *sport;
>
> - if (socket == NULL) {
> - rc = sock_create_kern(PF_INET6, SOCK_STREAM,
> - IPPROTO_TCP, &socket);
> - if (rc < 0) {
> - cERROR(1, "Error %d creating ipv6 socket", rc);
> - socket = NULL;
> - return rc;
> - }
> + if (server->addr.sockAddr6.sin6_family == AF_INET6)
> + sport = &server->addr.sockAddr6.sin6_port;
> + else
> + sport = &server->addr.sockAddr.sin_port;
>
> - /* BB other socket options to set KEEPALIVE, NODELAY? */
> - cFYI(1, "ipv6 Socket created");
> - server->ssocket = socket;
> - socket->sk->sk_allocation = GFP_NOFS;
> - cifs_reclassify_socket6(socket);
> - }
> + if (*sport == 0) {
> + int rc;
>
> - rc = bind_socket(server);
> - if (rc < 0)
> - return rc;
> + /* try with 445 port at first */
> + *sport = htons(CIFS_PORT);
>
> - /* user overrode default port */
> - if (server->addr.sockAddr6.sin6_port) {
> - rc = socket->ops->connect(socket,
> - (struct sockaddr *) &server->addr.sockAddr6,
> - sizeof(struct sockaddr_in6), 0);
> + rc = generic_ip_connect(server);
> if (rc >= 0)
> - connected = true;
> - }
> -
> - if (!connected) {
> - /* save original port so we can retry user specified port
> - later if fall back ports fail this time */
> -
> - orig_port = server->addr.sockAddr6.sin6_port;
> - /* do not retry on the same port we just failed on */
> - if (server->addr.sockAddr6.sin6_port != htons(CIFS_PORT)) {
> - server->addr.sockAddr6.sin6_port = htons(CIFS_PORT);
> - rc = socket->ops->connect(socket, (struct sockaddr *)
> - &server->addr.sockAddr6,
> - sizeof(struct sockaddr_in6), 0);
> - if (rc >= 0)
> - connected = true;
> - }
> - }
> - if (!connected) {
> - server->addr.sockAddr6.sin6_port = htons(RFC1001_PORT);
> - rc = socket->ops->connect(socket, (struct sockaddr *)
> - &server->addr.sockAddr6,
> - sizeof(struct sockaddr_in6), 0);
> - if (rc >= 0)
> - connected = true;
> - }
> -
> - /* give up here - unless we want to retry on different
> - protocol families some day */
> - if (!connected) {
> - if (orig_port)
> - server->addr.sockAddr6.sin6_port = orig_port;
> - cFYI(1, "Error %d connecting to server via ipv6", rc);
> - sock_release(socket);
> - server->ssocket = NULL;
> - return rc;
> - }
> -
> - /*
> - * Eventually check for other socket options to change from
> - * the default. sock_setsockopt not used because it expects
> - * user space buffer
> - */
> - socket->sk->sk_rcvtimeo = 7 * HZ;
> - socket->sk->sk_sndtimeo = 5 * HZ;
> + return rc;
>
> - if (server->tcp_nodelay) {
> - val = 1;
> - rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
> - (char *)&val, sizeof(val));
> - if (rc)
> - cFYI(1, "set TCP_NODELAY socket option error %d", rc);
> + /* if it failed, try with 139 port */
> + *sport = htons(RFC1001_PORT);
> }
>
> - server->ssocket = socket;
> -
> - return rc;
> + return generic_ip_connect(server);
> }
>
> void reset_cifs_unix_caps(int xid, struct cifsTconInfo *tcon,
Nice cleanup.
Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection (try #2)
[not found] ` <1289893710-7352-2-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2010-11-16 18:19 ` Jeff Layton
2010-11-16 18:47 ` Steve French
1 sibling, 0 replies; 16+ messages in thread
From: Jeff Layton @ 2010-11-16 18:19 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Tue, 16 Nov 2010 10:48:30 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> If we have a share mounted by non-standard port and try to mount another share
> on the same host with standard port, we connect to the first share again -
> that's wrong. This patch fixes this bug.
>
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> fs/cifs/connect.c | 42 ++++++++++++++++++++++++++++++++++++------
> 1 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 8bdf1cc..7956e7c 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1439,6 +1439,39 @@ srcip_matches(struct sockaddr *srcaddr, struct sockaddr *rhs)
> }
> }
>
> +/* If no port specified in addr structure, we try to match with 445 port
> + and if it fails - with 139 ports */
> +static bool
> +match_port(struct TCP_Server_Info *server, struct sockaddr *addr)
> +{
> + struct sockaddr_in *addr4 = (struct sockaddr_in *)addr;
> + struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)addr;
> + unsigned short int port, *sport;
> +
> + switch (addr->sa_family) {
> + case AF_INET:
> + sport = &server->addr.sockAddr.sin_port;
> + port = addr4->sin_port;
> + break;
> + case AF_INET6:
> + sport = &server->addr.sockAddr6.sin6_port;
> + port = addr6->sin6_port;
> + break;
> + default:
> + WARN_ON(1);
> + return false;
> + }
> +
> + if (!port) {
> + port = htons(CIFS_PORT);
> + if (port == *sport)
> + return true;
> +
> + port = htons(RFC1001_PORT);
> + }
> +
> + return port == *sport;
> +}
>
> static bool
> match_address(struct TCP_Server_Info *server, struct sockaddr *addr,
> @@ -1452,9 +1485,6 @@ match_address(struct TCP_Server_Info *server, struct sockaddr *addr,
> if (addr4->sin_addr.s_addr !=
> server->addr.sockAddr.sin_addr.s_addr)
> return false;
> - if (addr4->sin_port &&
> - addr4->sin_port != server->addr.sockAddr.sin_port)
> - return false;
> break;
> case AF_INET6:
> if (!ipv6_addr_equal(&addr6->sin6_addr,
> @@ -1463,9 +1493,6 @@ match_address(struct TCP_Server_Info *server, struct sockaddr *addr,
> if (addr6->sin6_scope_id !=
> server->addr.sockAddr6.sin6_scope_id)
> return false;
> - if (addr6->sin6_port &&
> - addr6->sin6_port != server->addr.sockAddr6.sin6_port)
> - return false;
> break;
> }
>
> @@ -1534,6 +1561,9 @@ cifs_find_tcp_session(struct sockaddr *addr, struct smb_vol *vol)
> (struct sockaddr *)&vol->srcaddr))
> continue;
>
> + if (!match_port(server, addr))
> + continue;
> +
> if (!match_security(server, vol))
> continue;
>
Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one (try #2)
[not found] ` <20101116131755.37043f53-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
@ 2010-11-16 18:25 ` Jeff Layton
[not found] ` <20101116132500.7865f16b-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
0 siblings, 1 reply; 16+ messages in thread
From: Jeff Layton @ 2010-11-16 18:25 UTC (permalink / raw)
To: Jeff Layton; +Cc: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Tue, 16 Nov 2010 13:17:55 -0500
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 16 Nov 2010 10:48:29 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> > Make connect logic more ip-protocol independent. Also move RFC1001 stuff into
> > separate function.
> >
> > Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > fs/cifs/connect.c | 314 ++++++++++++++++++++---------------------------------
> > 1 files changed, 116 insertions(+), 198 deletions(-)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 251a17c..8bdf1cc 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -114,8 +114,8 @@ struct smb_vol {
> > #define TLINK_ERROR_EXPIRE (1 * HZ)
> > #define TLINK_IDLE_EXPIRE (600 * HZ)
> >
> > -static int ipv4_connect(struct TCP_Server_Info *server);
> > -static int ipv6_connect(struct TCP_Server_Info *server);
> > +static int ip_connect(struct TCP_Server_Info *server);
> > +static int generic_ip_connect(struct TCP_Server_Info *server);
> > static void tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink);
> > static void cifs_prune_tlinks(struct work_struct *work);
> >
> > @@ -199,10 +199,10 @@ cifs_reconnect(struct TCP_Server_Info *server)
> > while ((server->tcpStatus != CifsExiting) &&
> > (server->tcpStatus != CifsGood)) {
> > try_to_freeze();
> > - if (server->addr.sockAddr6.sin6_family == AF_INET6)
> > - rc = ipv6_connect(server);
> > - else
> > - rc = ipv4_connect(server);
> > +
> > + /* on reconnect we should try only the port we connected
> > + before */
> > + rc = generic_ip_connect(server);
> > if (rc) {
> > cFYI(1, "reconnect error %d", rc);
> > msleep(3000);
> > @@ -1668,12 +1668,11 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
> > /* other OS never observed in Wild doing 139 with v6 */
> > memcpy(&tcp_ses->addr.sockAddr6, sin_server6,
> > sizeof(struct sockaddr_in6));
> > - rc = ipv6_connect(tcp_ses);
> > - } else {
> > + } else
> > memcpy(&tcp_ses->addr.sockAddr, sin_server,
> > sizeof(struct sockaddr_in));
> > - rc = ipv4_connect(tcp_ses);
> > - }
> > +
> > + rc = ip_connect(tcp_ses);
> > if (rc < 0) {
> > cERROR(1, "Error connecting to socket. Aborting operation");
> > goto out_err_crypto_release;
> > @@ -2121,19 +2120,97 @@ bind_socket(struct TCP_Server_Info *server)
> > }
> >
> > static int
> > -ipv4_connect(struct TCP_Server_Info *server)
> > +ip_rfc1001_connect(struct TCP_Server_Info *server)
> > +{
> > + int rc = 0;
> > + /* some servers require RFC1001 sessinit before sending
> > + negprot - BB check reconnection in case where second
> > + sessinit is sent but no second negprot */
> > + struct rfc1002_session_packet *ses_init_buf;
> > + struct smb_hdr *smb_buf;
> > + ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
> > + GFP_KERNEL);
> > + if (ses_init_buf) {
> > + ses_init_buf->trailer.session_req.called_len = 32;
> > +
> > + if (server->server_RFC1001_name &&
> > + server->server_RFC1001_name[0] != 0)
> > + rfc1002mangle(ses_init_buf->trailer.
> > + session_req.called_name,
> > + server->server_RFC1001_name,
> > + RFC1001_NAME_LEN_WITH_NULL);
> > + else
> > + rfc1002mangle(ses_init_buf->trailer.
> > + session_req.called_name,
> > + DEFAULT_CIFS_CALLED_NAME,
> > + RFC1001_NAME_LEN_WITH_NULL);
> > +
> > + ses_init_buf->trailer.session_req.calling_len = 32;
> > +
> > + /* calling name ends in null (byte 16) from old smb
> > + convention. */
> > + if (server->workstation_RFC1001_name &&
> > + server->workstation_RFC1001_name[0] != 0)
> > + rfc1002mangle(ses_init_buf->trailer.
> > + session_req.calling_name,
> > + server->workstation_RFC1001_name,
> > + RFC1001_NAME_LEN_WITH_NULL);
> > + else
> > + rfc1002mangle(ses_init_buf->trailer.
> > + session_req.calling_name,
> > + "LINUX_CIFS_CLNT",
> > + RFC1001_NAME_LEN_WITH_NULL);
> > +
> > + ses_init_buf->trailer.session_req.scope1 = 0;
> > + ses_init_buf->trailer.session_req.scope2 = 0;
> > + smb_buf = (struct smb_hdr *)ses_init_buf;
> > +
> > + /* sizeof RFC1002_SESSION_REQUEST with no scope */
> > + smb_buf->smb_buf_length = 0x81000044;
> > + rc = smb_send(server, smb_buf, 0x44);
> > + kfree(ses_init_buf);
> > + msleep(1); /* RFC1001 layer in at least one server
> > + requires very short break before negprot
> > + presumably because not expecting negprot
> > + to follow so fast. This is a simple
> > + solution that works without
> > + complicating the code and causes no
> > + significant slowing down on mount
> > + for everyone else */
> > + }
> > + /* else the negprot may still work without this
> > + even though malloc failed */
> > +
> > + return rc;
> > +}
> > +
> > +static int
> > +generic_ip_connect(struct TCP_Server_Info *server)
> > {
> > int rc = 0;
> > - int val;
> > - bool connected = false;
> > - __be16 orig_port = 0;
> > + bool using_ipv6 = false;
> > + unsigned short int *sport;
> > + int slen;
> > struct socket *socket = server->ssocket;
> > + struct sockaddr *saddr;
> > +
> > + if (server->addr.sockAddr6.sin6_family == AF_INET6) {
> > + sport = &server->addr.sockAddr6.sin6_port;
> > + using_ipv6 = true;
> > + slen = sizeof(struct sockaddr_in6);
> > + saddr = (struct sockaddr *) &server->addr.sockAddr6;
> > + } else {
> > + sport = &server->addr.sockAddr.sin_port;
> > + slen = sizeof(struct sockaddr_in);
> > + saddr = (struct sockaddr *) &server->addr.sockAddr;
> > + }
> >
> > if (socket == NULL) {
> > rc = sock_create_kern(PF_INET, SOCK_STREAM,
> > IPPROTO_TCP, &socket);
> > if (rc < 0) {
> > cERROR(1, "Error %d creating socket", rc);
> > + server->ssocket = NULL;
> > return rc;
> > }
> >
> > @@ -2141,59 +2218,24 @@ ipv4_connect(struct TCP_Server_Info *server)
> > cFYI(1, "Socket created");
> > server->ssocket = socket;
> > socket->sk->sk_allocation = GFP_NOFS;
> > - cifs_reclassify_socket4(socket);
> > + if (using_ipv6)
> > + cifs_reclassify_socket6(socket);
> > + else
> > + cifs_reclassify_socket4(socket);
> > }
> >
> > rc = bind_socket(server);
> > if (rc < 0)
> > return rc;
> >
> > - /* user overrode default port */
> > - if (server->addr.sockAddr.sin_port) {
> > - rc = socket->ops->connect(socket, (struct sockaddr *)
> > - &server->addr.sockAddr,
> > - sizeof(struct sockaddr_in), 0);
> > - if (rc >= 0)
> > - connected = true;
> > - }
> > -
> > - if (!connected) {
> > - /* save original port so we can retry user specified port
> > - later if fall back ports fail this time */
> > - orig_port = server->addr.sockAddr.sin_port;
> > -
> > - /* do not retry on the same port we just failed on */
> > - if (server->addr.sockAddr.sin_port != htons(CIFS_PORT)) {
> > - server->addr.sockAddr.sin_port = htons(CIFS_PORT);
> > - rc = socket->ops->connect(socket,
> > - (struct sockaddr *)
> > - &server->addr.sockAddr,
> > - sizeof(struct sockaddr_in), 0);
> > - if (rc >= 0)
> > - connected = true;
> > - }
> > - }
> > - if (!connected) {
> > - server->addr.sockAddr.sin_port = htons(RFC1001_PORT);
> > - rc = socket->ops->connect(socket, (struct sockaddr *)
> > - &server->addr.sockAddr,
> > - sizeof(struct sockaddr_in), 0);
> > - if (rc >= 0)
> > - connected = true;
> > - }
> > -
> > - /* give up here - unless we want to retry on different
> > - protocol families some day */
> > - if (!connected) {
> > - if (orig_port)
> > - server->addr.sockAddr.sin_port = orig_port;
> > - cFYI(1, "Error %d connecting to server via ipv4", rc);
> > + rc = socket->ops->connect(socket, saddr, slen, 0);
> > + if (rc < 0) {
> > + cFYI(1, "Error %d connecting to server", rc);
> > sock_release(socket);
> > server->ssocket = NULL;
> > return rc;
> > }
> >
> > -
> > /*
> > * Eventually check for other socket options to change from
> > * the default. sock_setsockopt not used because it expects
> > @@ -2211,7 +2253,7 @@ ipv4_connect(struct TCP_Server_Info *server)
> > }
> >
> > if (server->tcp_nodelay) {
> > - val = 1;
> > + int val = 1;
> > rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
> > (char *)&val, sizeof(val));
> > if (rc)
> > @@ -2222,161 +2264,37 @@ ipv4_connect(struct TCP_Server_Info *server)
> > socket->sk->sk_sndbuf,
> > socket->sk->sk_rcvbuf, socket->sk->sk_rcvtimeo);
> >
> > - /* send RFC1001 sessinit */
> > - if (server->addr.sockAddr.sin_port == htons(RFC1001_PORT)) {
> > - /* some servers require RFC1001 sessinit before sending
> > - negprot - BB check reconnection in case where second
> > - sessinit is sent but no second negprot */
> > - struct rfc1002_session_packet *ses_init_buf;
> > - struct smb_hdr *smb_buf;
> > - ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
> > - GFP_KERNEL);
> > - if (ses_init_buf) {
> > - ses_init_buf->trailer.session_req.called_len = 32;
> > - if (server->server_RFC1001_name &&
> > - server->server_RFC1001_name[0] != 0)
> > - rfc1002mangle(ses_init_buf->trailer.
> > - session_req.called_name,
> > - server->server_RFC1001_name,
> > - RFC1001_NAME_LEN_WITH_NULL);
> > - else
> > - rfc1002mangle(ses_init_buf->trailer.
> > - session_req.called_name,
> > - DEFAULT_CIFS_CALLED_NAME,
> > - RFC1001_NAME_LEN_WITH_NULL);
> > -
> > - ses_init_buf->trailer.session_req.calling_len = 32;
> > -
> > - /* calling name ends in null (byte 16) from old smb
> > - convention. */
> > - if (server->workstation_RFC1001_name &&
> > - server->workstation_RFC1001_name[0] != 0)
> > - rfc1002mangle(ses_init_buf->trailer.
> > - session_req.calling_name,
> > - server->workstation_RFC1001_name,
> > - RFC1001_NAME_LEN_WITH_NULL);
> > - else
> > - rfc1002mangle(ses_init_buf->trailer.
> > - session_req.calling_name,
> > - "LINUX_CIFS_CLNT",
> > - RFC1001_NAME_LEN_WITH_NULL);
> > -
> > - ses_init_buf->trailer.session_req.scope1 = 0;
> > - ses_init_buf->trailer.session_req.scope2 = 0;
> > - smb_buf = (struct smb_hdr *)ses_init_buf;
> > - /* sizeof RFC1002_SESSION_REQUEST with no scope */
> > - smb_buf->smb_buf_length = 0x81000044;
> > - rc = smb_send(server, smb_buf, 0x44);
> > - kfree(ses_init_buf);
> > - msleep(1); /* RFC1001 layer in at least one server
> > - requires very short break before negprot
> > - presumably because not expecting negprot
> > - to follow so fast. This is a simple
> > - solution that works without
> > - complicating the code and causes no
> > - significant slowing down on mount
> > - for everyone else */
> > - }
> > - /* else the negprot may still work without this
> > - even though malloc failed */
> > -
> > - }
> > + if (*sport == htons(RFC1001_PORT))
> > + rc = ip_rfc1001_connect(server);
> >
> > return rc;
> > }
> >
> > static int
> > -ipv6_connect(struct TCP_Server_Info *server)
> > +ip_connect(struct TCP_Server_Info *server)
> > {
> > - int rc = 0;
> > - int val;
> > - bool connected = false;
> > - __be16 orig_port = 0;
> > - struct socket *socket = server->ssocket;
> > + unsigned short int *sport;
> >
> > - if (socket == NULL) {
> > - rc = sock_create_kern(PF_INET6, SOCK_STREAM,
> > - IPPROTO_TCP, &socket);
> > - if (rc < 0) {
> > - cERROR(1, "Error %d creating ipv6 socket", rc);
> > - socket = NULL;
> > - return rc;
> > - }
> > + if (server->addr.sockAddr6.sin6_family == AF_INET6)
> > + sport = &server->addr.sockAddr6.sin6_port;
> > + else
> > + sport = &server->addr.sockAddr.sin_port;
> >
> > - /* BB other socket options to set KEEPALIVE, NODELAY? */
> > - cFYI(1, "ipv6 Socket created");
> > - server->ssocket = socket;
> > - socket->sk->sk_allocation = GFP_NOFS;
> > - cifs_reclassify_socket6(socket);
> > - }
> > + if (*sport == 0) {
> > + int rc;
> >
> > - rc = bind_socket(server);
> > - if (rc < 0)
> > - return rc;
> > + /* try with 445 port at first */
> > + *sport = htons(CIFS_PORT);
> >
> > - /* user overrode default port */
> > - if (server->addr.sockAddr6.sin6_port) {
> > - rc = socket->ops->connect(socket,
> > - (struct sockaddr *) &server->addr.sockAddr6,
> > - sizeof(struct sockaddr_in6), 0);
> > + rc = generic_ip_connect(server);
> > if (rc >= 0)
> > - connected = true;
> > - }
> > -
> > - if (!connected) {
> > - /* save original port so we can retry user specified port
> > - later if fall back ports fail this time */
> > -
> > - orig_port = server->addr.sockAddr6.sin6_port;
> > - /* do not retry on the same port we just failed on */
> > - if (server->addr.sockAddr6.sin6_port != htons(CIFS_PORT)) {
> > - server->addr.sockAddr6.sin6_port = htons(CIFS_PORT);
> > - rc = socket->ops->connect(socket, (struct sockaddr *)
> > - &server->addr.sockAddr6,
> > - sizeof(struct sockaddr_in6), 0);
> > - if (rc >= 0)
> > - connected = true;
> > - }
> > - }
> > - if (!connected) {
> > - server->addr.sockAddr6.sin6_port = htons(RFC1001_PORT);
> > - rc = socket->ops->connect(socket, (struct sockaddr *)
> > - &server->addr.sockAddr6,
> > - sizeof(struct sockaddr_in6), 0);
> > - if (rc >= 0)
> > - connected = true;
> > - }
> > -
> > - /* give up here - unless we want to retry on different
> > - protocol families some day */
> > - if (!connected) {
> > - if (orig_port)
> > - server->addr.sockAddr6.sin6_port = orig_port;
> > - cFYI(1, "Error %d connecting to server via ipv6", rc);
> > - sock_release(socket);
> > - server->ssocket = NULL;
> > - return rc;
> > - }
> > -
> > - /*
> > - * Eventually check for other socket options to change from
> > - * the default. sock_setsockopt not used because it expects
> > - * user space buffer
> > - */
> > - socket->sk->sk_rcvtimeo = 7 * HZ;
> > - socket->sk->sk_sndtimeo = 5 * HZ;
> > + return rc;
> >
> > - if (server->tcp_nodelay) {
> > - val = 1;
> > - rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
> > - (char *)&val, sizeof(val));
> > - if (rc)
> > - cFYI(1, "set TCP_NODELAY socket option error %d", rc);
> > + /* if it failed, try with 139 port */
> > + *sport = htons(RFC1001_PORT);
> > }
> >
> > - server->ssocket = socket;
> > -
> > - return rc;
> > + return generic_ip_connect(server);
> > }
> >
> > void reset_cifs_unix_caps(int xid, struct cifsTconInfo *tcon,
>
> Nice cleanup.
>
> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Actually...my ACK on this still stands if you don't want to do it, but
there is a "nice to have" change that could go with this.
It would be nice if we didn't have this goofy "addr" union in the
TCP_Server_Info, and just had a standard sockaddr_storage embedded
inside it instead. It would mean quite a bit less conditional code, I
think...
You're already touching about half the code that would be affected by
such a change with this patch. Care to do the rest?
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection (try #2)
[not found] ` <1289893710-7352-2-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-11-16 18:19 ` Jeff Layton
@ 2010-11-16 18:47 ` Steve French
[not found] ` <AANLkTinX6o59YD2EAL4ahJoD1PBLNUCF-MXUss9yhZnd-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 1 reply; 16+ messages in thread
From: Steve French @ 2010-11-16 18:47 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Tue, Nov 16, 2010 at 1:48 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> If we have a share mounted by non-standard port and try to mount another share
> on the same host with standard port, we connect to the first share again -
> that's wrong. This patch fixes this bug.
The description seems a little strange - can you clarify?
If we have an existing session with a server (using whatever port), by
default we want to use it (unless the user forces a different port
than the existing connection was made with). If we have a firewall
issue and therefore go through a non-standard port (port=something on
mount), we usually wouldn't want to fork a new connection on a second
connection to the same server.
So if we specify port=5000 on a mount and we already have a mount on
port 139, it makes sense that we would want to create a new session.
But if you already have a mount on port 5000, and did a mount with no
port specified - why wouldn't you use the existing port? It saves
server, client and network resources.
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection (try #2)
[not found] ` <AANLkTinX6o59YD2EAL4ahJoD1PBLNUCF-MXUss9yhZnd-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-11-16 18:57 ` Jeff Layton
[not found] ` <20101116135714.358d6b10-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2010-11-16 19:13 ` Pavel Shilovsky
1 sibling, 1 reply; 16+ messages in thread
From: Jeff Layton @ 2010-11-16 18:57 UTC (permalink / raw)
To: Steve French; +Cc: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Tue, 16 Nov 2010 12:47:07 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Nov 16, 2010 at 1:48 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > If we have a share mounted by non-standard port and try to mount another share
> > on the same host with standard port, we connect to the first share again -
> > that's wrong. This patch fixes this bug.
>
> The description seems a little strange - can you clarify?
>
> If we have an existing session with a server (using whatever port), by
> default we want to use it (unless the user forces a different port
> than the existing connection was made with). If we have a firewall
> issue and therefore go through a non-standard port (port=something on
> mount), we usually wouldn't want to fork a new connection on a second
> connection to the same server.
>
> So if we specify port=5000 on a mount and we already have a mount on
> port 139, it makes sense that we would want to create a new session.
> But if you already have a mount on port 5000, and did a mount with no
> port specified - why wouldn't you use the existing port? It saves
> server, client and network resources.
>
That doesn't match what the manpage says. It says:
port=arg
sets the port number on the server to attempt to contact to
negotiate CIFS support. If the CIFS server is not listening on this
port or if it is not specified, the default ports will be tried
i.e. port 445 is tried and if no response then port 139 is tried.
...which to me says that if you want the new mount to share the socket
already on port 5000, you need to specify port=5000 in the mount
options. The code doesn't do this today, but I think it probably ought
to conform to the above description, or more accurately, with the
manpage update that Pavel proposed.
Now, to be pedantic...the code that Pavel proposed still isn't 100%
compliant with the description. If someone specifies port=139 and the
server is also listening on 445, a second mount with no port= option
will end up using the socket on 139. Still, I think his patches are
good enough here and that corner case really isn't worth sweating over
too much.
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection (try #2)
[not found] ` <20101116135714.358d6b10-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
@ 2010-11-16 19:02 ` Steve French
[not found] ` <AANLkTimUVphDeWti-mid5jS2g-sF1jDVU7rMuyaPExUg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-11-16 19:22 ` Pavel Shilovsky
1 sibling, 1 reply; 16+ messages in thread
From: Steve French @ 2010-11-16 19:02 UTC (permalink / raw)
To: Jeff Layton; +Cc: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Tue, Nov 16, 2010 at 12:57 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 16 Nov 2010 12:47:07 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Tue, Nov 16, 2010 at 1:48 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8@public.gmane.orgm> wrote:
>> > If we have a share mounted by non-standard port and try to mount another share
>> > on the same host with standard port, we connect to the first share again -
>> > that's wrong. This patch fixes this bug.
>>
>> The description seems a little strange - can you clarify?
>>
>> If we have an existing session with a server (using whatever port), by
>> default we want to use it (unless the user forces a different port
>> than the existing connection was made with). If we have a firewall
>> issue and therefore go through a non-standard port (port=something on
>> mount), we usually wouldn't want to fork a new connection on a second
>> connection to the same server.
>>
>> So if we specify port=5000 on a mount and we already have a mount on
>> port 139, it makes sense that we would want to create a new session.
>> But if you already have a mount on port 5000, and did a mount with no
>> port specified - why wouldn't you use the existing port? It saves
>> server, client and network resources.
>>
>
> That doesn't match what the manpage says. It says:
>
> port=arg
> sets the port number on the server to attempt to contact to
> negotiate CIFS support. If the CIFS server is not listening on this
> port or if it is not specified, the default ports will be tried
> i.e. port 445 is tried and if no response then port 139 is tried.
>
>
> ...which to me says that if you want the new mount to share the socket
> already on port 5000, you need to specify port=5000 in the mount
> options. The code doesn't do this today, but I think it probably ought
> to conform to the above description, or more accurately, with the
> manpage update that Pavel proposed.
>
> Now, to be pedantic...the code that Pavel proposed still isn't 100%
> compliant with the description.
But I think that the more important question is what the use case is -
ie which is easier and more natural for the user. The man page needs
update either way.
Seems like we want to always use the existing connection if possible.
The man page comment about using the user specified port (if any) then
port 445 then port 139 is only for "negotiate" - ie when we haven't
already negotiated a session with the server.
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection (try #2)
[not found] ` <AANLkTinX6o59YD2EAL4ahJoD1PBLNUCF-MXUss9yhZnd-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-11-16 18:57 ` Jeff Layton
@ 2010-11-16 19:13 ` Pavel Shilovsky
1 sibling, 0 replies; 16+ messages in thread
From: Pavel Shilovsky @ 2010-11-16 19:13 UTC (permalink / raw)
To: Steve French; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
2010/11/16 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> On Tue, Nov 16, 2010 at 1:48 AM, Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> If we have a share mounted by non-standard port and try to mount another share
>> on the same host with standard port, we connect to the first share again -
>> that's wrong. This patch fixes this bug.
>
> The description seems a little strange - can you clarify?
>
> If we have an existing session with a server (using whatever port), by
> default we want to use it (unless the user forces a different port
> than the existing connection was made with). If we have a firewall
> issue and therefore go through a non-standard port (port=something on
> mount), we usually wouldn't want to fork a new connection on a second
> connection to the same server.
>
> So if we specify port=5000 on a mount and we already have a mount on
> port 139, it makes sense that we would want to create a new session.
> But if you already have a mount on port 5000, and did a mount with no
> port specified - why wouldn't you use the existing port? It saves
When we have several Samba servers on the same host on different
ports(e.x. 445 and 5000) and user has already mounted "5000" server,
we can't use the existing connection if it tries to mount again
without specifying the port, because user wants to mount the server on
445 port rather than 5000. They are dirrefent servers with different
shares and different contents and it doesn't matter that they are
listening on the same host - we have to established two TCP connection
anyway.
> server, client and network resources.
>
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection (try #2)
[not found] ` <AANLkTimUVphDeWti-mid5jS2g-sF1jDVU7rMuyaPExUg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-11-16 19:21 ` Jeff Layton
0 siblings, 0 replies; 16+ messages in thread
From: Jeff Layton @ 2010-11-16 19:21 UTC (permalink / raw)
To: Steve French; +Cc: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Tue, 16 Nov 2010 13:02:10 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Nov 16, 2010 at 12:57 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, 16 Nov 2010 12:47:07 -0600
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Tue, Nov 16, 2010 at 1:48 AM, Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >> > If we have a share mounted by non-standard port and try to mount another share
> >> > on the same host with standard port, we connect to the first share again -
> >> > that's wrong. This patch fixes this bug.
> >>
> >> The description seems a little strange - can you clarify?
> >>
> >> If we have an existing session with a server (using whatever port), by
> >> default we want to use it (unless the user forces a different port
> >> than the existing connection was made with). If we have a firewall
> >> issue and therefore go through a non-standard port (port=something on
> >> mount), we usually wouldn't want to fork a new connection on a second
> >> connection to the same server.
> >>
> >> So if we specify port=5000 on a mount and we already have a mount on
> >> port 139, it makes sense that we would want to create a new session.
> >> But if you already have a mount on port 5000, and did a mount with no
> >> port specified - why wouldn't you use the existing port? It saves
> >> server, client and network resources.
> >>
> >
> > That doesn't match what the manpage says. It says:
> >
> > port=arg
> > sets the port number on the server to attempt to contact to
> > negotiate CIFS support. If the CIFS server is not listening on this
> > port or if it is not specified, the default ports will be tried
> > i.e. port 445 is tried and if no response then port 139 is tried.
> >
> >
> > ...which to me says that if you want the new mount to share the socket
> > already on port 5000, you need to specify port=5000 in the mount
> > options. The code doesn't do this today, but I think it probably ought
> > to conform to the above description, or more accurately, with the
> > manpage update that Pavel proposed.
> >
> > Now, to be pedantic...the code that Pavel proposed still isn't 100%
> > compliant with the description.
>
> But I think that the more important question is what the use case is -
> ie which is easier and more natural for the user. The man page needs
> update either way.
>
> Seems like we want to always use the existing connection if possible.
> The man page comment about using the user specified port (if any) then
> port 445 then port 139 is only for "negotiate" - ie when we haven't
> already negotiated a session with the server.
>
>
The difference is this -- do you want lack of a port= option to mean "I
don't care what port I use, so autonegotiate if I don't have one
already", or should it mean "autonegotiate using standard cifs ports"?
The documentation says the latter and I think we ought to try to stick
to the documented behavior as closely as possible.
One problem with saying that lack of a port= option means "I don't
care" is that you can end up with entirely different results depending
on what order mounts are done. In your example, depending on the order
that those mounts are done, you'll end up with either one shared
socket, or two. If we go with Pavel's patch, then you'll get the same
result regardless of what order the mounts are done. That seems like a
better outcome to me.
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection (try #2)
[not found] ` <20101116135714.358d6b10-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2010-11-16 19:02 ` Steve French
@ 2010-11-16 19:22 ` Pavel Shilovsky
2010-11-16 19:29 ` Jeff Layton
1 sibling, 1 reply; 16+ messages in thread
From: Pavel Shilovsky @ 2010-11-16 19:22 UTC (permalink / raw)
To: Jeff Layton; +Cc: Steve French, linux-cifs-u79uwXL29TY76Z2rM5mHXA
2010/11/16 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>
> Now, to be pedantic...the code that Pavel proposed still isn't 100%
> compliant with the description. If someone specifies port=139 and the
> server is also listening on 445, a second mount with no port= option
> will end up using the socket on 139. Still, I think his patches are
> good enough here and that corner case really isn't worth sweating over
> too much.
Jeff, I think it fully follow the description I proposed today:
"port=arg
sets the port number on the server to attempt to contact to negotiate
CIFS support. If this value is specified, looking for an existing
connection with this port and try to connect if no such a connection.
Return an error if it fails.
If this value isn't specified, looking for an existing connection with
445 or 139 port. If no such a connection, try to connect with 445 port
and if it fails - with 139 port. Return an error if both fail."
So, in your case the second mount without port specifying will end up
with 139 and this is 100% compiliant with "If this value isn't
specified, looking for an existing connection with 445 or 139 port".
So, it means that servers on 445 and 139 ports can't live together on
the same host:)
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one (try #2)
[not found] ` <20101116132500.7865f16b-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
@ 2010-11-16 19:28 ` Pavel Shilovsky
0 siblings, 0 replies; 16+ messages in thread
From: Pavel Shilovsky @ 2010-11-16 19:28 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
2010/11/16 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Tue, 16 Nov 2010 13:17:55 -0500
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> On Tue, 16 Nov 2010 10:48:29 +0300
>> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> > Make connect logic more ip-protocol independent. Also move RFC1001 stuff into
>> > separate function.
>> >
>> > Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> > ---
>> > fs/cifs/connect.c | 314 ++++++++++++++++++++---------------------------------
>> > 1 files changed, 116 insertions(+), 198 deletions(-)
>> >
>> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> > index 251a17c..8bdf1cc 100644
>> > --- a/fs/cifs/connect.c
>> > +++ b/fs/cifs/connect.c
>> > @@ -114,8 +114,8 @@ struct smb_vol {
>> > #define TLINK_ERROR_EXPIRE (1 * HZ)
>> > #define TLINK_IDLE_EXPIRE (600 * HZ)
>> >
>> > -static int ipv4_connect(struct TCP_Server_Info *server);
>> > -static int ipv6_connect(struct TCP_Server_Info *server);
>> > +static int ip_connect(struct TCP_Server_Info *server);
>> > +static int generic_ip_connect(struct TCP_Server_Info *server);
>> > static void tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink);
>> > static void cifs_prune_tlinks(struct work_struct *work);
>> >
>> > @@ -199,10 +199,10 @@ cifs_reconnect(struct TCP_Server_Info *server)
>> > while ((server->tcpStatus != CifsExiting) &&
>> > (server->tcpStatus != CifsGood)) {
>> > try_to_freeze();
>> > - if (server->addr.sockAddr6.sin6_family == AF_INET6)
>> > - rc = ipv6_connect(server);
>> > - else
>> > - rc = ipv4_connect(server);
>> > +
>> > + /* on reconnect we should try only the port we connected
>> > + before */
>> > + rc = generic_ip_connect(server);
>> > if (rc) {
>> > cFYI(1, "reconnect error %d", rc);
>> > msleep(3000);
>> > @@ -1668,12 +1668,11 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>> > /* other OS never observed in Wild doing 139 with v6 */
>> > memcpy(&tcp_ses->addr.sockAddr6, sin_server6,
>> > sizeof(struct sockaddr_in6));
>> > - rc = ipv6_connect(tcp_ses);
>> > - } else {
>> > + } else
>> > memcpy(&tcp_ses->addr.sockAddr, sin_server,
>> > sizeof(struct sockaddr_in));
>> > - rc = ipv4_connect(tcp_ses);
>> > - }
>> > +
>> > + rc = ip_connect(tcp_ses);
>> > if (rc < 0) {
>> > cERROR(1, "Error connecting to socket. Aborting operation");
>> > goto out_err_crypto_release;
>> > @@ -2121,19 +2120,97 @@ bind_socket(struct TCP_Server_Info *server)
>> > }
>> >
>> > static int
>> > -ipv4_connect(struct TCP_Server_Info *server)
>> > +ip_rfc1001_connect(struct TCP_Server_Info *server)
>> > +{
>> > + int rc = 0;
>> > + /* some servers require RFC1001 sessinit before sending
>> > + negprot - BB check reconnection in case where second
>> > + sessinit is sent but no second negprot */
>> > + struct rfc1002_session_packet *ses_init_buf;
>> > + struct smb_hdr *smb_buf;
>> > + ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
>> > + GFP_KERNEL);
>> > + if (ses_init_buf) {
>> > + ses_init_buf->trailer.session_req.called_len = 32;
>> > +
>> > + if (server->server_RFC1001_name &&
>> > + server->server_RFC1001_name[0] != 0)
>> > + rfc1002mangle(ses_init_buf->trailer.
>> > + session_req.called_name,
>> > + server->server_RFC1001_name,
>> > + RFC1001_NAME_LEN_WITH_NULL);
>> > + else
>> > + rfc1002mangle(ses_init_buf->trailer.
>> > + session_req.called_name,
>> > + DEFAULT_CIFS_CALLED_NAME,
>> > + RFC1001_NAME_LEN_WITH_NULL);
>> > +
>> > + ses_init_buf->trailer.session_req.calling_len = 32;
>> > +
>> > + /* calling name ends in null (byte 16) from old smb
>> > + convention. */
>> > + if (server->workstation_RFC1001_name &&
>> > + server->workstation_RFC1001_name[0] != 0)
>> > + rfc1002mangle(ses_init_buf->trailer.
>> > + session_req.calling_name,
>> > + server->workstation_RFC1001_name,
>> > + RFC1001_NAME_LEN_WITH_NULL);
>> > + else
>> > + rfc1002mangle(ses_init_buf->trailer.
>> > + session_req.calling_name,
>> > + "LINUX_CIFS_CLNT",
>> > + RFC1001_NAME_LEN_WITH_NULL);
>> > +
>> > + ses_init_buf->trailer.session_req.scope1 = 0;
>> > + ses_init_buf->trailer.session_req.scope2 = 0;
>> > + smb_buf = (struct smb_hdr *)ses_init_buf;
>> > +
>> > + /* sizeof RFC1002_SESSION_REQUEST with no scope */
>> > + smb_buf->smb_buf_length = 0x81000044;
>> > + rc = smb_send(server, smb_buf, 0x44);
>> > + kfree(ses_init_buf);
>> > + msleep(1); /* RFC1001 layer in at least one server
>> > + requires very short break before negprot
>> > + presumably because not expecting negprot
>> > + to follow so fast. This is a simple
>> > + solution that works without
>> > + complicating the code and causes no
>> > + significant slowing down on mount
>> > + for everyone else */
>> > + }
>> > + /* else the negprot may still work without this
>> > + even though malloc failed */
>> > +
>> > + return rc;
>> > +}
>> > +
>> > +static int
>> > +generic_ip_connect(struct TCP_Server_Info *server)
>> > {
>> > int rc = 0;
>> > - int val;
>> > - bool connected = false;
>> > - __be16 orig_port = 0;
>> > + bool using_ipv6 = false;
>> > + unsigned short int *sport;
>> > + int slen;
>> > struct socket *socket = server->ssocket;
>> > + struct sockaddr *saddr;
>> > +
>> > + if (server->addr.sockAddr6.sin6_family == AF_INET6) {
>> > + sport = &server->addr.sockAddr6.sin6_port;
>> > + using_ipv6 = true;
>> > + slen = sizeof(struct sockaddr_in6);
>> > + saddr = (struct sockaddr *) &server->addr.sockAddr6;
>> > + } else {
>> > + sport = &server->addr.sockAddr.sin_port;
>> > + slen = sizeof(struct sockaddr_in);
>> > + saddr = (struct sockaddr *) &server->addr.sockAddr;
>> > + }
>> >
>> > if (socket == NULL) {
>> > rc = sock_create_kern(PF_INET, SOCK_STREAM,
>> > IPPROTO_TCP, &socket);
>> > if (rc < 0) {
>> > cERROR(1, "Error %d creating socket", rc);
>> > + server->ssocket = NULL;
>> > return rc;
>> > }
>> >
>> > @@ -2141,59 +2218,24 @@ ipv4_connect(struct TCP_Server_Info *server)
>> > cFYI(1, "Socket created");
>> > server->ssocket = socket;
>> > socket->sk->sk_allocation = GFP_NOFS;
>> > - cifs_reclassify_socket4(socket);
>> > + if (using_ipv6)
>> > + cifs_reclassify_socket6(socket);
>> > + else
>> > + cifs_reclassify_socket4(socket);
>> > }
>> >
>> > rc = bind_socket(server);
>> > if (rc < 0)
>> > return rc;
>> >
>> > - /* user overrode default port */
>> > - if (server->addr.sockAddr.sin_port) {
>> > - rc = socket->ops->connect(socket, (struct sockaddr *)
>> > - &server->addr.sockAddr,
>> > - sizeof(struct sockaddr_in), 0);
>> > - if (rc >= 0)
>> > - connected = true;
>> > - }
>> > -
>> > - if (!connected) {
>> > - /* save original port so we can retry user specified port
>> > - later if fall back ports fail this time */
>> > - orig_port = server->addr.sockAddr.sin_port;
>> > -
>> > - /* do not retry on the same port we just failed on */
>> > - if (server->addr.sockAddr.sin_port != htons(CIFS_PORT)) {
>> > - server->addr.sockAddr.sin_port = htons(CIFS_PORT);
>> > - rc = socket->ops->connect(socket,
>> > - (struct sockaddr *)
>> > - &server->addr.sockAddr,
>> > - sizeof(struct sockaddr_in), 0);
>> > - if (rc >= 0)
>> > - connected = true;
>> > - }
>> > - }
>> > - if (!connected) {
>> > - server->addr.sockAddr.sin_port = htons(RFC1001_PORT);
>> > - rc = socket->ops->connect(socket, (struct sockaddr *)
>> > - &server->addr.sockAddr,
>> > - sizeof(struct sockaddr_in), 0);
>> > - if (rc >= 0)
>> > - connected = true;
>> > - }
>> > -
>> > - /* give up here - unless we want to retry on different
>> > - protocol families some day */
>> > - if (!connected) {
>> > - if (orig_port)
>> > - server->addr.sockAddr.sin_port = orig_port;
>> > - cFYI(1, "Error %d connecting to server via ipv4", rc);
>> > + rc = socket->ops->connect(socket, saddr, slen, 0);
>> > + if (rc < 0) {
>> > + cFYI(1, "Error %d connecting to server", rc);
>> > sock_release(socket);
>> > server->ssocket = NULL;
>> > return rc;
>> > }
>> >
>> > -
>> > /*
>> > * Eventually check for other socket options to change from
>> > * the default. sock_setsockopt not used because it expects
>> > @@ -2211,7 +2253,7 @@ ipv4_connect(struct TCP_Server_Info *server)
>> > }
>> >
>> > if (server->tcp_nodelay) {
>> > - val = 1;
>> > + int val = 1;
>> > rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
>> > (char *)&val, sizeof(val));
>> > if (rc)
>> > @@ -2222,161 +2264,37 @@ ipv4_connect(struct TCP_Server_Info *server)
>> > socket->sk->sk_sndbuf,
>> > socket->sk->sk_rcvbuf, socket->sk->sk_rcvtimeo);
>> >
>> > - /* send RFC1001 sessinit */
>> > - if (server->addr.sockAddr.sin_port == htons(RFC1001_PORT)) {
>> > - /* some servers require RFC1001 sessinit before sending
>> > - negprot - BB check reconnection in case where second
>> > - sessinit is sent but no second negprot */
>> > - struct rfc1002_session_packet *ses_init_buf;
>> > - struct smb_hdr *smb_buf;
>> > - ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
>> > - GFP_KERNEL);
>> > - if (ses_init_buf) {
>> > - ses_init_buf->trailer.session_req.called_len = 32;
>> > - if (server->server_RFC1001_name &&
>> > - server->server_RFC1001_name[0] != 0)
>> > - rfc1002mangle(ses_init_buf->trailer.
>> > - session_req.called_name,
>> > - server->server_RFC1001_name,
>> > - RFC1001_NAME_LEN_WITH_NULL);
>> > - else
>> > - rfc1002mangle(ses_init_buf->trailer.
>> > - session_req.called_name,
>> > - DEFAULT_CIFS_CALLED_NAME,
>> > - RFC1001_NAME_LEN_WITH_NULL);
>> > -
>> > - ses_init_buf->trailer.session_req.calling_len = 32;
>> > -
>> > - /* calling name ends in null (byte 16) from old smb
>> > - convention. */
>> > - if (server->workstation_RFC1001_name &&
>> > - server->workstation_RFC1001_name[0] != 0)
>> > - rfc1002mangle(ses_init_buf->trailer.
>> > - session_req.calling_name,
>> > - server->workstation_RFC1001_name,
>> > - RFC1001_NAME_LEN_WITH_NULL);
>> > - else
>> > - rfc1002mangle(ses_init_buf->trailer.
>> > - session_req.calling_name,
>> > - "LINUX_CIFS_CLNT",
>> > - RFC1001_NAME_LEN_WITH_NULL);
>> > -
>> > - ses_init_buf->trailer.session_req.scope1 = 0;
>> > - ses_init_buf->trailer.session_req.scope2 = 0;
>> > - smb_buf = (struct smb_hdr *)ses_init_buf;
>> > - /* sizeof RFC1002_SESSION_REQUEST with no scope */
>> > - smb_buf->smb_buf_length = 0x81000044;
>> > - rc = smb_send(server, smb_buf, 0x44);
>> > - kfree(ses_init_buf);
>> > - msleep(1); /* RFC1001 layer in at least one server
>> > - requires very short break before negprot
>> > - presumably because not expecting negprot
>> > - to follow so fast. This is a simple
>> > - solution that works without
>> > - complicating the code and causes no
>> > - significant slowing down on mount
>> > - for everyone else */
>> > - }
>> > - /* else the negprot may still work without this
>> > - even though malloc failed */
>> > -
>> > - }
>> > + if (*sport == htons(RFC1001_PORT))
>> > + rc = ip_rfc1001_connect(server);
>> >
>> > return rc;
>> > }
>> >
>> > static int
>> > -ipv6_connect(struct TCP_Server_Info *server)
>> > +ip_connect(struct TCP_Server_Info *server)
>> > {
>> > - int rc = 0;
>> > - int val;
>> > - bool connected = false;
>> > - __be16 orig_port = 0;
>> > - struct socket *socket = server->ssocket;
>> > + unsigned short int *sport;
>> >
>> > - if (socket == NULL) {
>> > - rc = sock_create_kern(PF_INET6, SOCK_STREAM,
>> > - IPPROTO_TCP, &socket);
>> > - if (rc < 0) {
>> > - cERROR(1, "Error %d creating ipv6 socket", rc);
>> > - socket = NULL;
>> > - return rc;
>> > - }
>> > + if (server->addr.sockAddr6.sin6_family == AF_INET6)
>> > + sport = &server->addr.sockAddr6.sin6_port;
>> > + else
>> > + sport = &server->addr.sockAddr.sin_port;
>> >
>> > - /* BB other socket options to set KEEPALIVE, NODELAY? */
>> > - cFYI(1, "ipv6 Socket created");
>> > - server->ssocket = socket;
>> > - socket->sk->sk_allocation = GFP_NOFS;
>> > - cifs_reclassify_socket6(socket);
>> > - }
>> > + if (*sport == 0) {
>> > + int rc;
>> >
>> > - rc = bind_socket(server);
>> > - if (rc < 0)
>> > - return rc;
>> > + /* try with 445 port at first */
>> > + *sport = htons(CIFS_PORT);
>> >
>> > - /* user overrode default port */
>> > - if (server->addr.sockAddr6.sin6_port) {
>> > - rc = socket->ops->connect(socket,
>> > - (struct sockaddr *) &server->addr.sockAddr6,
>> > - sizeof(struct sockaddr_in6), 0);
>> > + rc = generic_ip_connect(server);
>> > if (rc >= 0)
>> > - connected = true;
>> > - }
>> > -
>> > - if (!connected) {
>> > - /* save original port so we can retry user specified port
>> > - later if fall back ports fail this time */
>> > -
>> > - orig_port = server->addr.sockAddr6.sin6_port;
>> > - /* do not retry on the same port we just failed on */
>> > - if (server->addr.sockAddr6.sin6_port != htons(CIFS_PORT)) {
>> > - server->addr.sockAddr6.sin6_port = htons(CIFS_PORT);
>> > - rc = socket->ops->connect(socket, (struct sockaddr *)
>> > - &server->addr.sockAddr6,
>> > - sizeof(struct sockaddr_in6), 0);
>> > - if (rc >= 0)
>> > - connected = true;
>> > - }
>> > - }
>> > - if (!connected) {
>> > - server->addr.sockAddr6.sin6_port = htons(RFC1001_PORT);
>> > - rc = socket->ops->connect(socket, (struct sockaddr *)
>> > - &server->addr.sockAddr6,
>> > - sizeof(struct sockaddr_in6), 0);
>> > - if (rc >= 0)
>> > - connected = true;
>> > - }
>> > -
>> > - /* give up here - unless we want to retry on different
>> > - protocol families some day */
>> > - if (!connected) {
>> > - if (orig_port)
>> > - server->addr.sockAddr6.sin6_port = orig_port;
>> > - cFYI(1, "Error %d connecting to server via ipv6", rc);
>> > - sock_release(socket);
>> > - server->ssocket = NULL;
>> > - return rc;
>> > - }
>> > -
>> > - /*
>> > - * Eventually check for other socket options to change from
>> > - * the default. sock_setsockopt not used because it expects
>> > - * user space buffer
>> > - */
>> > - socket->sk->sk_rcvtimeo = 7 * HZ;
>> > - socket->sk->sk_sndtimeo = 5 * HZ;
>> > + return rc;
>> >
>> > - if (server->tcp_nodelay) {
>> > - val = 1;
>> > - rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
>> > - (char *)&val, sizeof(val));
>> > - if (rc)
>> > - cFYI(1, "set TCP_NODELAY socket option error %d", rc);
>> > + /* if it failed, try with 139 port */
>> > + *sport = htons(RFC1001_PORT);
>> > }
>> >
>> > - server->ssocket = socket;
>> > -
>> > - return rc;
>> > + return generic_ip_connect(server);
>> > }
>> >
>> > void reset_cifs_unix_caps(int xid, struct cifsTconInfo *tcon,
>>
>> Nice cleanup.
>>
>> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> Actually...my ACK on this still stands if you don't want to do it, but
> there is a "nice to have" change that could go with this.
>
> It would be nice if we didn't have this goofy "addr" union in the
> TCP_Server_Info, and just had a standard sockaddr_storage embedded
> inside it instead. It would mean quite a bit less conditional code, I
> think...
>
> You're already touching about half the code that would be affected by
> such a change with this patch. Care to do the rest?
Ok, good idea. But I think it should be another separate patch that we
can apply after the patch above.
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection (try #2)
2010-11-16 19:22 ` Pavel Shilovsky
@ 2010-11-16 19:29 ` Jeff Layton
0 siblings, 0 replies; 16+ messages in thread
From: Jeff Layton @ 2010-11-16 19:29 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Steve French, linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Tue, 16 Nov 2010 22:22:40 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2010/11/16 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>
> >
> > Now, to be pedantic...the code that Pavel proposed still isn't 100%
> > compliant with the description. If someone specifies port=139 and the
> > server is also listening on 445, a second mount with no port= option
> > will end up using the socket on 139. Still, I think his patches are
> > good enough here and that corner case really isn't worth sweating over
> > too much.
>
> Jeff, I think it fully follow the description I proposed today:
>
> "port=arg
>
> sets the port number on the server to attempt to contact to negotiate
> CIFS support. If this value is specified, looking for an existing
> connection with this port and try to connect if no such a connection.
> Return an error if it fails.
>
> If this value isn't specified, looking for an existing connection with
> 445 or 139 port. If no such a connection, try to connect with 445 port
> and if it fails - with 139 port. Return an error if both fail."
>
> So, in your case the second mount without port specifying will end up
> with 139 and this is 100% compiliant with "If this value isn't
> specified, looking for an existing connection with 445 or 139 port".
>
> So, it means that servers on 445 and 139 ports can't live together on
> the same host:)
>
True. That manpage change would fully describe the behavior with your
patch in place. FWIW, it's common for servers to listen on both
ports.
The thing you won't be able to expect is to autonegotiate to 445
once you've got a socket connected to 139 on the same server. It
wouldn't be too hard to make that happen, but it's probably not worth
it.
Cheers,
--
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one (try #2)
[not found] ` <1289893710-7352-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-11-16 7:48 ` [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection " Pavel Shilovsky
2010-11-16 18:17 ` [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one " Jeff Layton
@ 2010-11-22 17:01 ` Jeff Layton
[not found] ` <20101122120145.20989d38-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2 siblings, 1 reply; 16+ messages in thread
From: Jeff Layton @ 2010-11-22 17:01 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Tue, 16 Nov 2010 10:48:29 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Make connect logic more ip-protocol independent. Also move RFC1001 stuff into
> separate function.
>
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> fs/cifs/connect.c | 314 ++++++++++++++++++++---------------------------------
> 1 files changed, 116 insertions(+), 198 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 251a17c..8bdf1cc 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -114,8 +114,8 @@ struct smb_vol {
> #define TLINK_ERROR_EXPIRE (1 * HZ)
> #define TLINK_IDLE_EXPIRE (600 * HZ)
>
> -static int ipv4_connect(struct TCP_Server_Info *server);
> -static int ipv6_connect(struct TCP_Server_Info *server);
> +static int ip_connect(struct TCP_Server_Info *server);
> +static int generic_ip_connect(struct TCP_Server_Info *server);
> static void tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink);
> static void cifs_prune_tlinks(struct work_struct *work);
>
> @@ -199,10 +199,10 @@ cifs_reconnect(struct TCP_Server_Info *server)
> while ((server->tcpStatus != CifsExiting) &&
> (server->tcpStatus != CifsGood)) {
> try_to_freeze();
> - if (server->addr.sockAddr6.sin6_family == AF_INET6)
> - rc = ipv6_connect(server);
> - else
> - rc = ipv4_connect(server);
> +
> + /* on reconnect we should try only the port we connected
> + before */
> + rc = generic_ip_connect(server);
> if (rc) {
> cFYI(1, "reconnect error %d", rc);
> msleep(3000);
> @@ -1668,12 +1668,11 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
> /* other OS never observed in Wild doing 139 with v6 */
> memcpy(&tcp_ses->addr.sockAddr6, sin_server6,
> sizeof(struct sockaddr_in6));
> - rc = ipv6_connect(tcp_ses);
> - } else {
> + } else
> memcpy(&tcp_ses->addr.sockAddr, sin_server,
> sizeof(struct sockaddr_in));
> - rc = ipv4_connect(tcp_ses);
> - }
> +
> + rc = ip_connect(tcp_ses);
> if (rc < 0) {
> cERROR(1, "Error connecting to socket. Aborting operation");
> goto out_err_crypto_release;
> @@ -2121,19 +2120,97 @@ bind_socket(struct TCP_Server_Info *server)
> }
>
> static int
> -ipv4_connect(struct TCP_Server_Info *server)
> +ip_rfc1001_connect(struct TCP_Server_Info *server)
> +{
> + int rc = 0;
> + /* some servers require RFC1001 sessinit before sending
> + negprot - BB check reconnection in case where second
> + sessinit is sent but no second negprot */
> + struct rfc1002_session_packet *ses_init_buf;
> + struct smb_hdr *smb_buf;
> + ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
> + GFP_KERNEL);
> + if (ses_init_buf) {
> + ses_init_buf->trailer.session_req.called_len = 32;
> +
> + if (server->server_RFC1001_name &&
> + server->server_RFC1001_name[0] != 0)
> + rfc1002mangle(ses_init_buf->trailer.
> + session_req.called_name,
> + server->server_RFC1001_name,
> + RFC1001_NAME_LEN_WITH_NULL);
> + else
> + rfc1002mangle(ses_init_buf->trailer.
> + session_req.called_name,
> + DEFAULT_CIFS_CALLED_NAME,
> + RFC1001_NAME_LEN_WITH_NULL);
> +
> + ses_init_buf->trailer.session_req.calling_len = 32;
> +
> + /* calling name ends in null (byte 16) from old smb
> + convention. */
> + if (server->workstation_RFC1001_name &&
> + server->workstation_RFC1001_name[0] != 0)
> + rfc1002mangle(ses_init_buf->trailer.
> + session_req.calling_name,
> + server->workstation_RFC1001_name,
> + RFC1001_NAME_LEN_WITH_NULL);
> + else
> + rfc1002mangle(ses_init_buf->trailer.
> + session_req.calling_name,
> + "LINUX_CIFS_CLNT",
> + RFC1001_NAME_LEN_WITH_NULL);
> +
> + ses_init_buf->trailer.session_req.scope1 = 0;
> + ses_init_buf->trailer.session_req.scope2 = 0;
> + smb_buf = (struct smb_hdr *)ses_init_buf;
> +
> + /* sizeof RFC1002_SESSION_REQUEST with no scope */
> + smb_buf->smb_buf_length = 0x81000044;
> + rc = smb_send(server, smb_buf, 0x44);
> + kfree(ses_init_buf);
> + msleep(1); /* RFC1001 layer in at least one server
> + requires very short break before negprot
> + presumably because not expecting negprot
> + to follow so fast. This is a simple
> + solution that works without
> + complicating the code and causes no
> + significant slowing down on mount
> + for everyone else */
> + }
> + /* else the negprot may still work without this
> + even though malloc failed */
> +
> + return rc;
> +}
> +
> +static int
> +generic_ip_connect(struct TCP_Server_Info *server)
> {
> int rc = 0;
> - int val;
> - bool connected = false;
> - __be16 orig_port = 0;
> + bool using_ipv6 = false;
> + unsigned short int *sport;
> + int slen;
> struct socket *socket = server->ssocket;
> + struct sockaddr *saddr;
> +
> + if (server->addr.sockAddr6.sin6_family == AF_INET6) {
> + sport = &server->addr.sockAddr6.sin6_port;
> + using_ipv6 = true;
> + slen = sizeof(struct sockaddr_in6);
> + saddr = (struct sockaddr *) &server->addr.sockAddr6;
> + } else {
> + sport = &server->addr.sockAddr.sin_port;
> + slen = sizeof(struct sockaddr_in);
> + saddr = (struct sockaddr *) &server->addr.sockAddr;
> + }
>
> if (socket == NULL) {
> rc = sock_create_kern(PF_INET, SOCK_STREAM,
> IPPROTO_TCP, &socket);
^^^^^
I finally got a chance to give this a try today. It's broken due to the
above statement. If we get an IPv6 address, we need to set the family
correctly in sock_create_kern.
Pavel, care to fix this?
> if (rc < 0) {
> cERROR(1, "Error %d creating socket", rc);
> + server->ssocket = NULL;
> return rc;
> }
>
> @@ -2141,59 +2218,24 @@ ipv4_connect(struct TCP_Server_Info *server)
> cFYI(1, "Socket created");
> server->ssocket = socket;
> socket->sk->sk_allocation = GFP_NOFS;
> - cifs_reclassify_socket4(socket);
> + if (using_ipv6)
> + cifs_reclassify_socket6(socket);
> + else
> + cifs_reclassify_socket4(socket);
> }
>
> rc = bind_socket(server);
> if (rc < 0)
> return rc;
>
> - /* user overrode default port */
> - if (server->addr.sockAddr.sin_port) {
> - rc = socket->ops->connect(socket, (struct sockaddr *)
> - &server->addr.sockAddr,
> - sizeof(struct sockaddr_in), 0);
> - if (rc >= 0)
> - connected = true;
> - }
> -
> - if (!connected) {
> - /* save original port so we can retry user specified port
> - later if fall back ports fail this time */
> - orig_port = server->addr.sockAddr.sin_port;
> -
> - /* do not retry on the same port we just failed on */
> - if (server->addr.sockAddr.sin_port != htons(CIFS_PORT)) {
> - server->addr.sockAddr.sin_port = htons(CIFS_PORT);
> - rc = socket->ops->connect(socket,
> - (struct sockaddr *)
> - &server->addr.sockAddr,
> - sizeof(struct sockaddr_in), 0);
> - if (rc >= 0)
> - connected = true;
> - }
> - }
> - if (!connected) {
> - server->addr.sockAddr.sin_port = htons(RFC1001_PORT);
> - rc = socket->ops->connect(socket, (struct sockaddr *)
> - &server->addr.sockAddr,
> - sizeof(struct sockaddr_in), 0);
> - if (rc >= 0)
> - connected = true;
> - }
> -
> - /* give up here - unless we want to retry on different
> - protocol families some day */
> - if (!connected) {
> - if (orig_port)
> - server->addr.sockAddr.sin_port = orig_port;
> - cFYI(1, "Error %d connecting to server via ipv4", rc);
> + rc = socket->ops->connect(socket, saddr, slen, 0);
> + if (rc < 0) {
> + cFYI(1, "Error %d connecting to server", rc);
> sock_release(socket);
> server->ssocket = NULL;
> return rc;
> }
>
> -
> /*
> * Eventually check for other socket options to change from
> * the default. sock_setsockopt not used because it expects
> @@ -2211,7 +2253,7 @@ ipv4_connect(struct TCP_Server_Info *server)
> }
>
> if (server->tcp_nodelay) {
> - val = 1;
> + int val = 1;
> rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
> (char *)&val, sizeof(val));
> if (rc)
> @@ -2222,161 +2264,37 @@ ipv4_connect(struct TCP_Server_Info *server)
> socket->sk->sk_sndbuf,
> socket->sk->sk_rcvbuf, socket->sk->sk_rcvtimeo);
>
> - /* send RFC1001 sessinit */
> - if (server->addr.sockAddr.sin_port == htons(RFC1001_PORT)) {
> - /* some servers require RFC1001 sessinit before sending
> - negprot - BB check reconnection in case where second
> - sessinit is sent but no second negprot */
> - struct rfc1002_session_packet *ses_init_buf;
> - struct smb_hdr *smb_buf;
> - ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
> - GFP_KERNEL);
> - if (ses_init_buf) {
> - ses_init_buf->trailer.session_req.called_len = 32;
> - if (server->server_RFC1001_name &&
> - server->server_RFC1001_name[0] != 0)
> - rfc1002mangle(ses_init_buf->trailer.
> - session_req.called_name,
> - server->server_RFC1001_name,
> - RFC1001_NAME_LEN_WITH_NULL);
> - else
> - rfc1002mangle(ses_init_buf->trailer.
> - session_req.called_name,
> - DEFAULT_CIFS_CALLED_NAME,
> - RFC1001_NAME_LEN_WITH_NULL);
> -
> - ses_init_buf->trailer.session_req.calling_len = 32;
> -
> - /* calling name ends in null (byte 16) from old smb
> - convention. */
> - if (server->workstation_RFC1001_name &&
> - server->workstation_RFC1001_name[0] != 0)
> - rfc1002mangle(ses_init_buf->trailer.
> - session_req.calling_name,
> - server->workstation_RFC1001_name,
> - RFC1001_NAME_LEN_WITH_NULL);
> - else
> - rfc1002mangle(ses_init_buf->trailer.
> - session_req.calling_name,
> - "LINUX_CIFS_CLNT",
> - RFC1001_NAME_LEN_WITH_NULL);
> -
> - ses_init_buf->trailer.session_req.scope1 = 0;
> - ses_init_buf->trailer.session_req.scope2 = 0;
> - smb_buf = (struct smb_hdr *)ses_init_buf;
> - /* sizeof RFC1002_SESSION_REQUEST with no scope */
> - smb_buf->smb_buf_length = 0x81000044;
> - rc = smb_send(server, smb_buf, 0x44);
> - kfree(ses_init_buf);
> - msleep(1); /* RFC1001 layer in at least one server
> - requires very short break before negprot
> - presumably because not expecting negprot
> - to follow so fast. This is a simple
> - solution that works without
> - complicating the code and causes no
> - significant slowing down on mount
> - for everyone else */
> - }
> - /* else the negprot may still work without this
> - even though malloc failed */
> -
> - }
> + if (*sport == htons(RFC1001_PORT))
> + rc = ip_rfc1001_connect(server);
>
> return rc;
> }
>
> static int
> -ipv6_connect(struct TCP_Server_Info *server)
> +ip_connect(struct TCP_Server_Info *server)
> {
> - int rc = 0;
> - int val;
> - bool connected = false;
> - __be16 orig_port = 0;
> - struct socket *socket = server->ssocket;
> + unsigned short int *sport;
>
> - if (socket == NULL) {
> - rc = sock_create_kern(PF_INET6, SOCK_STREAM,
> - IPPROTO_TCP, &socket);
> - if (rc < 0) {
> - cERROR(1, "Error %d creating ipv6 socket", rc);
> - socket = NULL;
> - return rc;
> - }
> + if (server->addr.sockAddr6.sin6_family == AF_INET6)
> + sport = &server->addr.sockAddr6.sin6_port;
> + else
> + sport = &server->addr.sockAddr.sin_port;
>
> - /* BB other socket options to set KEEPALIVE, NODELAY? */
> - cFYI(1, "ipv6 Socket created");
> - server->ssocket = socket;
> - socket->sk->sk_allocation = GFP_NOFS;
> - cifs_reclassify_socket6(socket);
> - }
> + if (*sport == 0) {
> + int rc;
>
> - rc = bind_socket(server);
> - if (rc < 0)
> - return rc;
> + /* try with 445 port at first */
> + *sport = htons(CIFS_PORT);
>
> - /* user overrode default port */
> - if (server->addr.sockAddr6.sin6_port) {
> - rc = socket->ops->connect(socket,
> - (struct sockaddr *) &server->addr.sockAddr6,
> - sizeof(struct sockaddr_in6), 0);
> + rc = generic_ip_connect(server);
> if (rc >= 0)
> - connected = true;
> - }
> -
> - if (!connected) {
> - /* save original port so we can retry user specified port
> - later if fall back ports fail this time */
> -
> - orig_port = server->addr.sockAddr6.sin6_port;
> - /* do not retry on the same port we just failed on */
> - if (server->addr.sockAddr6.sin6_port != htons(CIFS_PORT)) {
> - server->addr.sockAddr6.sin6_port = htons(CIFS_PORT);
> - rc = socket->ops->connect(socket, (struct sockaddr *)
> - &server->addr.sockAddr6,
> - sizeof(struct sockaddr_in6), 0);
> - if (rc >= 0)
> - connected = true;
> - }
> - }
> - if (!connected) {
> - server->addr.sockAddr6.sin6_port = htons(RFC1001_PORT);
> - rc = socket->ops->connect(socket, (struct sockaddr *)
> - &server->addr.sockAddr6,
> - sizeof(struct sockaddr_in6), 0);
> - if (rc >= 0)
> - connected = true;
> - }
> -
> - /* give up here - unless we want to retry on different
> - protocol families some day */
> - if (!connected) {
> - if (orig_port)
> - server->addr.sockAddr6.sin6_port = orig_port;
> - cFYI(1, "Error %d connecting to server via ipv6", rc);
> - sock_release(socket);
> - server->ssocket = NULL;
> - return rc;
> - }
> -
> - /*
> - * Eventually check for other socket options to change from
> - * the default. sock_setsockopt not used because it expects
> - * user space buffer
> - */
> - socket->sk->sk_rcvtimeo = 7 * HZ;
> - socket->sk->sk_sndtimeo = 5 * HZ;
> + return rc;
>
> - if (server->tcp_nodelay) {
> - val = 1;
> - rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
> - (char *)&val, sizeof(val));
> - if (rc)
> - cFYI(1, "set TCP_NODELAY socket option error %d", rc);
> + /* if it failed, try with 139 port */
> + *sport = htons(RFC1001_PORT);
> }
>
> - server->ssocket = socket;
> -
> - return rc;
> + return generic_ip_connect(server);
> }
>
> void reset_cifs_unix_caps(int xid, struct cifsTconInfo *tcon,
--
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one (try #2)
[not found] ` <20101122120145.20989d38-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
@ 2010-11-22 19:26 ` Pavel Shilovsky
[not found] ` <AANLkTi=V89tV62iyD+_DeSjwDBBVTfyfHMJ6t1mtBrCD-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 16+ messages in thread
From: Pavel Shilovsky @ 2010-11-22 19:26 UTC (permalink / raw)
To: Jeff Layton, Steve French; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA
2010/11/22 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> On Tue, 16 Nov 2010 10:48:29 +0300
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Make connect logic more ip-protocol independent. Also move RFC1001 stuff into
>> separate function.
>>
>> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> fs/cifs/connect.c | 314 ++++++++++++++++++++---------------------------------
>> 1 files changed, 116 insertions(+), 198 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 251a17c..8bdf1cc 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -114,8 +114,8 @@ struct smb_vol {
>> #define TLINK_ERROR_EXPIRE (1 * HZ)
>> #define TLINK_IDLE_EXPIRE (600 * HZ)
>>
>> -static int ipv4_connect(struct TCP_Server_Info *server);
>> -static int ipv6_connect(struct TCP_Server_Info *server);
>> +static int ip_connect(struct TCP_Server_Info *server);
>> +static int generic_ip_connect(struct TCP_Server_Info *server);
>> static void tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink);
>> static void cifs_prune_tlinks(struct work_struct *work);
>>
>> @@ -199,10 +199,10 @@ cifs_reconnect(struct TCP_Server_Info *server)
>> while ((server->tcpStatus != CifsExiting) &&
>> (server->tcpStatus != CifsGood)) {
>> try_to_freeze();
>> - if (server->addr.sockAddr6.sin6_family == AF_INET6)
>> - rc = ipv6_connect(server);
>> - else
>> - rc = ipv4_connect(server);
>> +
>> + /* on reconnect we should try only the port we connected
>> + before */
>> + rc = generic_ip_connect(server);
>> if (rc) {
>> cFYI(1, "reconnect error %d", rc);
>> msleep(3000);
>> @@ -1668,12 +1668,11 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
>> /* other OS never observed in Wild doing 139 with v6 */
>> memcpy(&tcp_ses->addr.sockAddr6, sin_server6,
>> sizeof(struct sockaddr_in6));
>> - rc = ipv6_connect(tcp_ses);
>> - } else {
>> + } else
>> memcpy(&tcp_ses->addr.sockAddr, sin_server,
>> sizeof(struct sockaddr_in));
>> - rc = ipv4_connect(tcp_ses);
>> - }
>> +
>> + rc = ip_connect(tcp_ses);
>> if (rc < 0) {
>> cERROR(1, "Error connecting to socket. Aborting operation");
>> goto out_err_crypto_release;
>> @@ -2121,19 +2120,97 @@ bind_socket(struct TCP_Server_Info *server)
>> }
>>
>> static int
>> -ipv4_connect(struct TCP_Server_Info *server)
>> +ip_rfc1001_connect(struct TCP_Server_Info *server)
>> +{
>> + int rc = 0;
>> + /* some servers require RFC1001 sessinit before sending
>> + negprot - BB check reconnection in case where second
>> + sessinit is sent but no second negprot */
>> + struct rfc1002_session_packet *ses_init_buf;
>> + struct smb_hdr *smb_buf;
>> + ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
>> + GFP_KERNEL);
>> + if (ses_init_buf) {
>> + ses_init_buf->trailer.session_req.called_len = 32;
>> +
>> + if (server->server_RFC1001_name &&
>> + server->server_RFC1001_name[0] != 0)
>> + rfc1002mangle(ses_init_buf->trailer.
>> + session_req.called_name,
>> + server->server_RFC1001_name,
>> + RFC1001_NAME_LEN_WITH_NULL);
>> + else
>> + rfc1002mangle(ses_init_buf->trailer.
>> + session_req.called_name,
>> + DEFAULT_CIFS_CALLED_NAME,
>> + RFC1001_NAME_LEN_WITH_NULL);
>> +
>> + ses_init_buf->trailer.session_req.calling_len = 32;
>> +
>> + /* calling name ends in null (byte 16) from old smb
>> + convention. */
>> + if (server->workstation_RFC1001_name &&
>> + server->workstation_RFC1001_name[0] != 0)
>> + rfc1002mangle(ses_init_buf->trailer.
>> + session_req.calling_name,
>> + server->workstation_RFC1001_name,
>> + RFC1001_NAME_LEN_WITH_NULL);
>> + else
>> + rfc1002mangle(ses_init_buf->trailer.
>> + session_req.calling_name,
>> + "LINUX_CIFS_CLNT",
>> + RFC1001_NAME_LEN_WITH_NULL);
>> +
>> + ses_init_buf->trailer.session_req.scope1 = 0;
>> + ses_init_buf->trailer.session_req.scope2 = 0;
>> + smb_buf = (struct smb_hdr *)ses_init_buf;
>> +
>> + /* sizeof RFC1002_SESSION_REQUEST with no scope */
>> + smb_buf->smb_buf_length = 0x81000044;
>> + rc = smb_send(server, smb_buf, 0x44);
>> + kfree(ses_init_buf);
>> + msleep(1); /* RFC1001 layer in at least one server
>> + requires very short break before negprot
>> + presumably because not expecting negprot
>> + to follow so fast. This is a simple
>> + solution that works without
>> + complicating the code and causes no
>> + significant slowing down on mount
>> + for everyone else */
>> + }
>> + /* else the negprot may still work without this
>> + even though malloc failed */
>> +
>> + return rc;
>> +}
>> +
>> +static int
>> +generic_ip_connect(struct TCP_Server_Info *server)
>> {
>> int rc = 0;
>> - int val;
>> - bool connected = false;
>> - __be16 orig_port = 0;
>> + bool using_ipv6 = false;
>> + unsigned short int *sport;
>> + int slen;
>> struct socket *socket = server->ssocket;
>> + struct sockaddr *saddr;
>> +
>> + if (server->addr.sockAddr6.sin6_family == AF_INET6) {
>> + sport = &server->addr.sockAddr6.sin6_port;
>> + using_ipv6 = true;
>> + slen = sizeof(struct sockaddr_in6);
>> + saddr = (struct sockaddr *) &server->addr.sockAddr6;
>> + } else {
>> + sport = &server->addr.sockAddr.sin_port;
>> + slen = sizeof(struct sockaddr_in);
>> + saddr = (struct sockaddr *) &server->addr.sockAddr;
>> + }
>>
>> if (socket == NULL) {
>> rc = sock_create_kern(PF_INET, SOCK_STREAM,
>> IPPROTO_TCP, &socket);
>
> ^^^^^
> I finally got a chance to give this a try today. It's broken due to the
> above statement. If we get an IPv6 address, we need to set the family
> correctly in sock_create_kern.
>
> Pavel, care to fix this?
No problem. Should I create separate patch or change this one?
>
>> if (rc < 0) {
>> cERROR(1, "Error %d creating socket", rc);
>> + server->ssocket = NULL;
>> return rc;
>> }
>>
>> @@ -2141,59 +2218,24 @@ ipv4_connect(struct TCP_Server_Info *server)
>> cFYI(1, "Socket created");
>> server->ssocket = socket;
>> socket->sk->sk_allocation = GFP_NOFS;
>> - cifs_reclassify_socket4(socket);
>> + if (using_ipv6)
>> + cifs_reclassify_socket6(socket);
>> + else
>> + cifs_reclassify_socket4(socket);
>> }
>>
>> rc = bind_socket(server);
>> if (rc < 0)
>> return rc;
>>
>> - /* user overrode default port */
>> - if (server->addr.sockAddr.sin_port) {
>> - rc = socket->ops->connect(socket, (struct sockaddr *)
>> - &server->addr.sockAddr,
>> - sizeof(struct sockaddr_in), 0);
>> - if (rc >= 0)
>> - connected = true;
>> - }
>> -
>> - if (!connected) {
>> - /* save original port so we can retry user specified port
>> - later if fall back ports fail this time */
>> - orig_port = server->addr.sockAddr.sin_port;
>> -
>> - /* do not retry on the same port we just failed on */
>> - if (server->addr.sockAddr.sin_port != htons(CIFS_PORT)) {
>> - server->addr.sockAddr.sin_port = htons(CIFS_PORT);
>> - rc = socket->ops->connect(socket,
>> - (struct sockaddr *)
>> - &server->addr.sockAddr,
>> - sizeof(struct sockaddr_in), 0);
>> - if (rc >= 0)
>> - connected = true;
>> - }
>> - }
>> - if (!connected) {
>> - server->addr.sockAddr.sin_port = htons(RFC1001_PORT);
>> - rc = socket->ops->connect(socket, (struct sockaddr *)
>> - &server->addr.sockAddr,
>> - sizeof(struct sockaddr_in), 0);
>> - if (rc >= 0)
>> - connected = true;
>> - }
>> -
>> - /* give up here - unless we want to retry on different
>> - protocol families some day */
>> - if (!connected) {
>> - if (orig_port)
>> - server->addr.sockAddr.sin_port = orig_port;
>> - cFYI(1, "Error %d connecting to server via ipv4", rc);
>> + rc = socket->ops->connect(socket, saddr, slen, 0);
>> + if (rc < 0) {
>> + cFYI(1, "Error %d connecting to server", rc);
>> sock_release(socket);
>> server->ssocket = NULL;
>> return rc;
>> }
>>
>> -
>> /*
>> * Eventually check for other socket options to change from
>> * the default. sock_setsockopt not used because it expects
>> @@ -2211,7 +2253,7 @@ ipv4_connect(struct TCP_Server_Info *server)
>> }
>>
>> if (server->tcp_nodelay) {
>> - val = 1;
>> + int val = 1;
>> rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
>> (char *)&val, sizeof(val));
>> if (rc)
>> @@ -2222,161 +2264,37 @@ ipv4_connect(struct TCP_Server_Info *server)
>> socket->sk->sk_sndbuf,
>> socket->sk->sk_rcvbuf, socket->sk->sk_rcvtimeo);
>>
>> - /* send RFC1001 sessinit */
>> - if (server->addr.sockAddr.sin_port == htons(RFC1001_PORT)) {
>> - /* some servers require RFC1001 sessinit before sending
>> - negprot - BB check reconnection in case where second
>> - sessinit is sent but no second negprot */
>> - struct rfc1002_session_packet *ses_init_buf;
>> - struct smb_hdr *smb_buf;
>> - ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
>> - GFP_KERNEL);
>> - if (ses_init_buf) {
>> - ses_init_buf->trailer.session_req.called_len = 32;
>> - if (server->server_RFC1001_name &&
>> - server->server_RFC1001_name[0] != 0)
>> - rfc1002mangle(ses_init_buf->trailer.
>> - session_req.called_name,
>> - server->server_RFC1001_name,
>> - RFC1001_NAME_LEN_WITH_NULL);
>> - else
>> - rfc1002mangle(ses_init_buf->trailer.
>> - session_req.called_name,
>> - DEFAULT_CIFS_CALLED_NAME,
>> - RFC1001_NAME_LEN_WITH_NULL);
>> -
>> - ses_init_buf->trailer.session_req.calling_len = 32;
>> -
>> - /* calling name ends in null (byte 16) from old smb
>> - convention. */
>> - if (server->workstation_RFC1001_name &&
>> - server->workstation_RFC1001_name[0] != 0)
>> - rfc1002mangle(ses_init_buf->trailer.
>> - session_req.calling_name,
>> - server->workstation_RFC1001_name,
>> - RFC1001_NAME_LEN_WITH_NULL);
>> - else
>> - rfc1002mangle(ses_init_buf->trailer.
>> - session_req.calling_name,
>> - "LINUX_CIFS_CLNT",
>> - RFC1001_NAME_LEN_WITH_NULL);
>> -
>> - ses_init_buf->trailer.session_req.scope1 = 0;
>> - ses_init_buf->trailer.session_req.scope2 = 0;
>> - smb_buf = (struct smb_hdr *)ses_init_buf;
>> - /* sizeof RFC1002_SESSION_REQUEST with no scope */
>> - smb_buf->smb_buf_length = 0x81000044;
>> - rc = smb_send(server, smb_buf, 0x44);
>> - kfree(ses_init_buf);
>> - msleep(1); /* RFC1001 layer in at least one server
>> - requires very short break before negprot
>> - presumably because not expecting negprot
>> - to follow so fast. This is a simple
>> - solution that works without
>> - complicating the code and causes no
>> - significant slowing down on mount
>> - for everyone else */
>> - }
>> - /* else the negprot may still work without this
>> - even though malloc failed */
>> -
>> - }
>> + if (*sport == htons(RFC1001_PORT))
>> + rc = ip_rfc1001_connect(server);
>>
>> return rc;
>> }
>>
>> static int
>> -ipv6_connect(struct TCP_Server_Info *server)
>> +ip_connect(struct TCP_Server_Info *server)
>> {
>> - int rc = 0;
>> - int val;
>> - bool connected = false;
>> - __be16 orig_port = 0;
>> - struct socket *socket = server->ssocket;
>> + unsigned short int *sport;
>>
>> - if (socket == NULL) {
>> - rc = sock_create_kern(PF_INET6, SOCK_STREAM,
>> - IPPROTO_TCP, &socket);
>> - if (rc < 0) {
>> - cERROR(1, "Error %d creating ipv6 socket", rc);
>> - socket = NULL;
>> - return rc;
>> - }
>> + if (server->addr.sockAddr6.sin6_family == AF_INET6)
>> + sport = &server->addr.sockAddr6.sin6_port;
>> + else
>> + sport = &server->addr.sockAddr.sin_port;
>>
>> - /* BB other socket options to set KEEPALIVE, NODELAY? */
>> - cFYI(1, "ipv6 Socket created");
>> - server->ssocket = socket;
>> - socket->sk->sk_allocation = GFP_NOFS;
>> - cifs_reclassify_socket6(socket);
>> - }
>> + if (*sport == 0) {
>> + int rc;
>>
>> - rc = bind_socket(server);
>> - if (rc < 0)
>> - return rc;
>> + /* try with 445 port at first */
>> + *sport = htons(CIFS_PORT);
>>
>> - /* user overrode default port */
>> - if (server->addr.sockAddr6.sin6_port) {
>> - rc = socket->ops->connect(socket,
>> - (struct sockaddr *) &server->addr.sockAddr6,
>> - sizeof(struct sockaddr_in6), 0);
>> + rc = generic_ip_connect(server);
>> if (rc >= 0)
>> - connected = true;
>> - }
>> -
>> - if (!connected) {
>> - /* save original port so we can retry user specified port
>> - later if fall back ports fail this time */
>> -
>> - orig_port = server->addr.sockAddr6.sin6_port;
>> - /* do not retry on the same port we just failed on */
>> - if (server->addr.sockAddr6.sin6_port != htons(CIFS_PORT)) {
>> - server->addr.sockAddr6.sin6_port = htons(CIFS_PORT);
>> - rc = socket->ops->connect(socket, (struct sockaddr *)
>> - &server->addr.sockAddr6,
>> - sizeof(struct sockaddr_in6), 0);
>> - if (rc >= 0)
>> - connected = true;
>> - }
>> - }
>> - if (!connected) {
>> - server->addr.sockAddr6.sin6_port = htons(RFC1001_PORT);
>> - rc = socket->ops->connect(socket, (struct sockaddr *)
>> - &server->addr.sockAddr6,
>> - sizeof(struct sockaddr_in6), 0);
>> - if (rc >= 0)
>> - connected = true;
>> - }
>> -
>> - /* give up here - unless we want to retry on different
>> - protocol families some day */
>> - if (!connected) {
>> - if (orig_port)
>> - server->addr.sockAddr6.sin6_port = orig_port;
>> - cFYI(1, "Error %d connecting to server via ipv6", rc);
>> - sock_release(socket);
>> - server->ssocket = NULL;
>> - return rc;
>> - }
>> -
>> - /*
>> - * Eventually check for other socket options to change from
>> - * the default. sock_setsockopt not used because it expects
>> - * user space buffer
>> - */
>> - socket->sk->sk_rcvtimeo = 7 * HZ;
>> - socket->sk->sk_sndtimeo = 5 * HZ;
>> + return rc;
>>
>> - if (server->tcp_nodelay) {
>> - val = 1;
>> - rc = kernel_setsockopt(socket, SOL_TCP, TCP_NODELAY,
>> - (char *)&val, sizeof(val));
>> - if (rc)
>> - cFYI(1, "set TCP_NODELAY socket option error %d", rc);
>> + /* if it failed, try with 139 port */
>> + *sport = htons(RFC1001_PORT);
>> }
>>
>> - server->ssocket = socket;
>> -
>> - return rc;
>> + return generic_ip_connect(server);
>> }
>>
>> void reset_cifs_unix_caps(int xid, struct cifsTconInfo *tcon,
>
>
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>
--
Best regards,
Pavel Shilovsky.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one (try #2)
[not found] ` <AANLkTi=V89tV62iyD+_DeSjwDBBVTfyfHMJ6t1mtBrCD-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-11-22 19:51 ` Jeff Layton
0 siblings, 0 replies; 16+ messages in thread
From: Jeff Layton @ 2010-11-22 19:51 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Steve French, linux-cifs-u79uwXL29TY76Z2rM5mHXA
On Mon, 22 Nov 2010 22:26:59 +0300
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2010/11/22 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > On Tue, 16 Nov 2010 10:48:29 +0300
> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> Make connect logic more ip-protocol independent. Also move RFC1001 stuff into
> >> separate function.
> >>
> >> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >> fs/cifs/connect.c | 314 ++++++++++++++++++++---------------------------------
> >> 1 files changed, 116 insertions(+), 198 deletions(-)
> >>
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index 251a17c..8bdf1cc 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -114,8 +114,8 @@ struct smb_vol {
> >> #define TLINK_ERROR_EXPIRE (1 * HZ)
> >> #define TLINK_IDLE_EXPIRE (600 * HZ)
> >>
> >> -static int ipv4_connect(struct TCP_Server_Info *server);
> >> -static int ipv6_connect(struct TCP_Server_Info *server);
> >> +static int ip_connect(struct TCP_Server_Info *server);
> >> +static int generic_ip_connect(struct TCP_Server_Info *server);
> >> static void tlink_rb_insert(struct rb_root *root, struct tcon_link *new_tlink);
> >> static void cifs_prune_tlinks(struct work_struct *work);
> >>
> >> @@ -199,10 +199,10 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >> while ((server->tcpStatus != CifsExiting) &&
> >> (server->tcpStatus != CifsGood)) {
> >> try_to_freeze();
> >> - if (server->addr.sockAddr6.sin6_family == AF_INET6)
> >> - rc = ipv6_connect(server);
> >> - else
> >> - rc = ipv4_connect(server);
> >> +
> >> + /* on reconnect we should try only the port we connected
> >> + before */
> >> + rc = generic_ip_connect(server);
> >> if (rc) {
> >> cFYI(1, "reconnect error %d", rc);
> >> msleep(3000);
> >> @@ -1668,12 +1668,11 @@ cifs_get_tcp_session(struct smb_vol *volume_info)
> >> /* other OS never observed in Wild doing 139 with v6 */
> >> memcpy(&tcp_ses->addr.sockAddr6, sin_server6,
> >> sizeof(struct sockaddr_in6));
> >> - rc = ipv6_connect(tcp_ses);
> >> - } else {
> >> + } else
> >> memcpy(&tcp_ses->addr.sockAddr, sin_server,
> >> sizeof(struct sockaddr_in));
> >> - rc = ipv4_connect(tcp_ses);
> >> - }
> >> +
> >> + rc = ip_connect(tcp_ses);
> >> if (rc < 0) {
> >> cERROR(1, "Error connecting to socket. Aborting operation");
> >> goto out_err_crypto_release;
> >> @@ -2121,19 +2120,97 @@ bind_socket(struct TCP_Server_Info *server)
> >> }
> >>
> >> static int
> >> -ipv4_connect(struct TCP_Server_Info *server)
> >> +ip_rfc1001_connect(struct TCP_Server_Info *server)
> >> +{
> >> + int rc = 0;
> >> + /* some servers require RFC1001 sessinit before sending
> >> + negprot - BB check reconnection in case where second
> >> + sessinit is sent but no second negprot */
> >> + struct rfc1002_session_packet *ses_init_buf;
> >> + struct smb_hdr *smb_buf;
> >> + ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
> >> + GFP_KERNEL);
> >> + if (ses_init_buf) {
> >> + ses_init_buf->trailer.session_req.called_len = 32;
> >> +
> >> + if (server->server_RFC1001_name &&
> >> + server->server_RFC1001_name[0] != 0)
> >> + rfc1002mangle(ses_init_buf->trailer.
> >> + session_req.called_name,
> >> + server->server_RFC1001_name,
> >> + RFC1001_NAME_LEN_WITH_NULL);
> >> + else
> >> + rfc1002mangle(ses_init_buf->trailer.
> >> + session_req.called_name,
> >> + DEFAULT_CIFS_CALLED_NAME,
> >> + RFC1001_NAME_LEN_WITH_NULL);
> >> +
> >> + ses_init_buf->trailer.session_req.calling_len = 32;
> >> +
> >> + /* calling name ends in null (byte 16) from old smb
> >> + convention. */
> >> + if (server->workstation_RFC1001_name &&
> >> + server->workstation_RFC1001_name[0] != 0)
> >> + rfc1002mangle(ses_init_buf->trailer.
> >> + session_req.calling_name,
> >> + server->workstation_RFC1001_name,
> >> + RFC1001_NAME_LEN_WITH_NULL);
> >> + else
> >> + rfc1002mangle(ses_init_buf->trailer.
> >> + session_req.calling_name,
> >> + "LINUX_CIFS_CLNT",
> >> + RFC1001_NAME_LEN_WITH_NULL);
> >> +
> >> + ses_init_buf->trailer.session_req.scope1 = 0;
> >> + ses_init_buf->trailer.session_req.scope2 = 0;
> >> + smb_buf = (struct smb_hdr *)ses_init_buf;
> >> +
> >> + /* sizeof RFC1002_SESSION_REQUEST with no scope */
> >> + smb_buf->smb_buf_length = 0x81000044;
> >> + rc = smb_send(server, smb_buf, 0x44);
> >> + kfree(ses_init_buf);
> >> + msleep(1); /* RFC1001 layer in at least one server
> >> + requires very short break before negprot
> >> + presumably because not expecting negprot
> >> + to follow so fast. This is a simple
> >> + solution that works without
> >> + complicating the code and causes no
> >> + significant slowing down on mount
> >> + for everyone else */
> >> + }
> >> + /* else the negprot may still work without this
> >> + even though malloc failed */
> >> +
> >> + return rc;
> >> +}
> >> +
> >> +static int
> >> +generic_ip_connect(struct TCP_Server_Info *server)
> >> {
> >> int rc = 0;
> >> - int val;
> >> - bool connected = false;
> >> - __be16 orig_port = 0;
> >> + bool using_ipv6 = false;
> >> + unsigned short int *sport;
> >> + int slen;
> >> struct socket *socket = server->ssocket;
> >> + struct sockaddr *saddr;
> >> +
> >> + if (server->addr.sockAddr6.sin6_family == AF_INET6) {
> >> + sport = &server->addr.sockAddr6.sin6_port;
> >> + using_ipv6 = true;
> >> + slen = sizeof(struct sockaddr_in6);
> >> + saddr = (struct sockaddr *) &server->addr.sockAddr6;
> >> + } else {
> >> + sport = &server->addr.sockAddr.sin_port;
> >> + slen = sizeof(struct sockaddr_in);
> >> + saddr = (struct sockaddr *) &server->addr.sockAddr;
> >> + }
> >>
> >> if (socket == NULL) {
> >> rc = sock_create_kern(PF_INET, SOCK_STREAM,
> >> IPPROTO_TCP, &socket);
> >
> > ^^^^^
> > I finally got a chance to give this a try today. It's broken due to the
> > above statement. If we get an IPv6 address, we need to set the family
> > correctly in sock_create_kern.
> >
> > Pavel, care to fix this?
>
> No problem. Should I create separate patch or change this one?
Please fix this one. I don't believe Steve has merged it yet.
--
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2010-11-22 19:51 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-16 7:48 [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one (try #2) Pavel Shilovsky
[not found] ` <1289893710-7352-1-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-11-16 7:48 ` [PATCH 2/2] CIFS: Add match_port check during looking for an existing connection " Pavel Shilovsky
[not found] ` <1289893710-7352-2-git-send-email-piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2010-11-16 18:19 ` Jeff Layton
2010-11-16 18:47 ` Steve French
[not found] ` <AANLkTinX6o59YD2EAL4ahJoD1PBLNUCF-MXUss9yhZnd-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-11-16 18:57 ` Jeff Layton
[not found] ` <20101116135714.358d6b10-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2010-11-16 19:02 ` Steve French
[not found] ` <AANLkTimUVphDeWti-mid5jS2g-sF1jDVU7rMuyaPExUg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-11-16 19:21 ` Jeff Layton
2010-11-16 19:22 ` Pavel Shilovsky
2010-11-16 19:29 ` Jeff Layton
2010-11-16 19:13 ` Pavel Shilovsky
2010-11-16 18:17 ` [PATCH 1/2] CIFS: Simplify ipv*_connect functions into one " Jeff Layton
[not found] ` <20101116131755.37043f53-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2010-11-16 18:25 ` Jeff Layton
[not found] ` <20101116132500.7865f16b-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2010-11-16 19:28 ` Pavel Shilovsky
2010-11-22 17:01 ` Jeff Layton
[not found] ` <20101122120145.20989d38-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2010-11-22 19:26 ` Pavel Shilovsky
[not found] ` <AANLkTi=V89tV62iyD+_DeSjwDBBVTfyfHMJ6t1mtBrCD-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-11-22 19:51 ` Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox