tipc-discussion — User questions & problem reports, project news, developer discussion

[tipc-discussion] [PATCH net-next] tipc: fix cancellation of topology subscriptions

[<eri...@gm...>]

From: Erik Hugne <eri...@gm...>

When cancelling a subscription, we have to clear the cancel bit
in the request before iterating over any established subscriptions
with memcmp. Otherwise no subscription will ever be found, and it
will not be possible to unsubscribe.

Signed-off-by: Erik Hugne <eri...@gm...>
---
Ignore my previous garbage patch, this solve the problem with one line.
Looking through the history of topsrv.c, i cannot find that this
has ever worked.

 net/tipc/topsrv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
index 4a708a4e8583..b05a89a723b1 100644
--- a/net/tipc/topsrv.c
+++ b/net/tipc/topsrv.c
@@ -363,6 +363,7 @@ static int tipc_conn_rcv_sub(struct tipc_topsrv *srv,
 	struct tipc_subscription *sub;
 
 	if (tipc_sub_read(s, filter) & TIPC_SUB_CANCEL) {
+		s->filter ^= __constant_ntohl(TIPC_SUB_CANCEL);
 		tipc_conn_delete_sub(con, s);
 		return 0;
 	}
-- 
2.14.1

[tipc-discussion] Subscription cancellation is broken

[Erik H. <eri...@gm...>]

Hi
a raw memcmp of the subscriptions wont work here:
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/net/tipc/topsrv.c#n236

The subscription in the list may have filter set to 0x0, 0x1 or 0x2, but the
received cancellation request this will always be 0x4
The result is that no sub will be found when processing the cancel request,
and events will be generated until you close the topology connection.
I made a real hacky workaround here:
https://gist.github.com/Hugne/18f900fccb0fd606093ca294a1c5b961

Re: [tipc-discussion] [net v2 1/1] tipc: allow service ranges to be connect()'ed on RDM/DGRAM

[David M. <da...@da...>]

From: Jon Maloy <jon...@er...>
Date: Sun, 17 Mar 2019 18:46:42 +0100

> From: Erik Hugne <eri...@gm...>
> 
> We move the check that prevents connecting service ranges to after
> the RDM/DGRAM check, and move address sanity control to a separate
> function that also validates the service range.
> 
> Fixes: 23998835be98 ("tipc: improve address sanity check in tipc_connect()")
> Signed-off-by: Erik Hugne <eri...@gm...>
> Signed-off-by: Jon Maloy <jon...@er...>
> ---
> v2: address check function returns bool as suggested by davem

Applied and queued up for -stable, thanks Jon.

[tipc-discussion] [net v2 1/1] tipc: allow service ranges to be connect()'ed on RDM/DGRAM

[Jon M. <jon...@er...>]

From: Erik Hugne <eri...@gm...>

We move the check that prevents connecting service ranges to after
the RDM/DGRAM check, and move address sanity control to a separate
function that also validates the service range.

Fixes: 23998835be98 ("tipc: improve address sanity check in tipc_connect()")
Signed-off-by: Erik Hugne <eri...@gm...>
Signed-off-by: Jon Maloy <jon...@er...>
---
v2: address check function returns bool as suggested by davem
---
 net/tipc/socket.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index d6b2686..b542f14 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2349,6 +2349,16 @@ static int tipc_wait_for_connect(struct socket *sock, long *timeo_p)
 	return 0;
 }
 
+static bool tipc_sockaddr_is_sane(struct sockaddr_tipc *addr)
+{
+	if (addr->family != AF_TIPC)
+		return false;
+	if (addr->addrtype == TIPC_SERVICE_RANGE)
+		return (addr->addr.nameseq.lower <= addr->addr.nameseq.upper);
+	return (addr->addrtype == TIPC_SERVICE_ADDR ||
+		addr->addrtype == TIPC_SOCKET_ADDR);
+}
+
 /**
  * tipc_connect - establish a connection to another TIPC port
  * @sock: socket structure
@@ -2384,18 +2394,18 @@ static int tipc_connect(struct socket *sock, struct sockaddr *dest,
 		if (!tipc_sk_type_connectionless(sk))
 			res = -EINVAL;
 		goto exit;
-	} else if (dst->family != AF_TIPC) {
-		res = -EINVAL;
 	}
-	if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype != TIPC_ADDR_NAME)
+	if (!tipc_sockaddr_is_sane(dst)) {
 		res = -EINVAL;
-	if (res)
 		goto exit;
-
+	}
 	/* DGRAM/RDM connect(), just save the destaddr */
 	if (tipc_sk_type_connectionless(sk)) {
 		memcpy(&tsk->peer, dest, destlen);
 		goto exit;
+	} else if (dst->addrtype == TIPC_SERVICE_RANGE) {
+		res = -EINVAL;
+		goto exit;
 	}
 
 	previous = sk->sk_state;
-- 
2.1.4

[tipc-discussion] [PATCH v3] tipc: allow service ranges to be connect()'ed on RDM/DGRAM

[<eri...@gm...>]

From: Erik Hugne <eri...@gm...>

We move the check that prevents connecting service service ranges
to after the RDM/DGRAM check, and split out address sanitizing to
a separate function that also validates the service range.

Fixes: 23998835be98 ("tipc: improve address sanity check in tipc_connect()")
Signed-off-by: Erik Hugne <eri...@gm...>
---
v3: address check function returns bool as suggested by davem

 net/tipc/socket.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 3274ef625dba..0f678e92fd30 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2349,6 +2349,16 @@ static int tipc_wait_for_connect(struct socket *sock, long *timeo_p)
 	return 0;
 }
 
+static bool tipc_sockaddr_is_sane(struct sockaddr_tipc *addr)
+{
+	if (addr->family != AF_TIPC)
+		return false;
+	if (addr->addrtype == TIPC_SERVICE_RANGE)
+		return (addr->addr.nameseq.lower <= addr->addr.nameseq.upper);
+	return (addr->addrtype == TIPC_SERVICE_ADDR ||
+		addr->addrtype == TIPC_SOCKET_ADDR);
+}
+
 /**
  * tipc_connect - establish a connection to another TIPC port
  * @sock: socket structure
@@ -2384,18 +2394,18 @@ static int tipc_connect(struct socket *sock, struct sockaddr *dest,
 		if (!tipc_sk_type_connectionless(sk))
 			res = -EINVAL;
 		goto exit;
-	} else if (dst->family != AF_TIPC) {
-		res = -EINVAL;
 	}
-	if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype != TIPC_ADDR_NAME)
+	if (!tipc_sockaddr_is_sane(dst)) {
 		res = -EINVAL;
-	if (res)
 		goto exit;
-
+	}
 	/* DGRAM/RDM connect(), just save the destaddr */
 	if (tipc_sk_type_connectionless(sk)) {
 		memcpy(&tsk->peer, dest, destlen);
 		goto exit;
+	} else if (dst->addrtype == TIPC_SERVICE_RANGE) {
+		res = -EINVAL;
+		goto exit;
 	}
 
 	previous = sk->sk_state;
-- 
2.14.1

Re: [tipc-discussion] [PATCH] net: tipc: fix a missing check for nla_nest_start

[David M. <da...@da...>]

From: Kangjie Lu <kj...@um...>
Date: Sat, 16 Mar 2019 16:46:05 -0500

> nla_nest_start may fail. The fix check its status and returns
> -EMSGSIZE in case it fails.
> 
> Signed-off-by: Kangjie Lu <kj...@um...>

Applied, thanks.

Re: [tipc-discussion] [net 1/1] tipc: allow service ranges to be connect()'ed on RDM/DGRAM

[David M. <da...@da...>]

From: Jon Maloy <jon...@er...>
Date: Fri, 15 Mar 2019 21:11:00 +0100

> @@ -2349,6 +2349,16 @@ static int tipc_wait_for_connect(struct socket *sock, long *timeo_p)
>  	return 0;
>  }
>  
> +static int tipc_sockaddr_is_sane(struct sockaddr_tipc *addr)
> +{
> +	if (addr->family != AF_TIPC)
> +		return 0;

I think bool and true/false are most appropriate for this function.

Re: [tipc-discussion] [PATCH] net: tipc: fix a missing check of nla_nest_start

[David M. <da...@da...>]

From: Kangjie Lu <kj...@um...>
Date: Fri, 15 Mar 2019 12:11:59 -0500

> nla_nest_start could fail and requires a check. The fix returns
> -EMSGSIZE if it fails.
> 
> Signed-off-by: Kangjie Lu <kj...@um...>

Applied.

Re: [tipc-discussion] [PATCH] net: socket: fix a missing check for nla_nest_start

[David M. <da...@da...>]

From: Kangjie Lu <kj...@um...>
Date: Fri, 15 Mar 2019 01:03:05 -0500

> nla_nest_start may fail. The fix check its status and returns
> -EMSGSIZE in case it fails.
> 
> Signed-off-by: Kangjie Lu <kj...@um...>

The proper subsystem prefix is "tipc: " not "socket: "

[tipc-discussion] [net 1/1] tipc: allow service ranges to be connect()'ed on RDM/DGRAM

[Jon M. <jon...@er...>]

From: Erik Hugne <eri...@gm...>

We move the check that prevents connecting service ranges to after
the RDM/DGRAM check, and separate address sanity control to a
separate function that also validates the service range.

Fixes: 23998835be98 ("tipc: improve address sanity check in tipc_connect()")
Signed-off-by: Erik Hugne <eri...@gm...>
Signed-off-by: Jon Maloy <jon...@er...>
---
 net/tipc/socket.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 3274ef6..1950781 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2349,6 +2349,16 @@ static int tipc_wait_for_connect(struct socket *sock, long *timeo_p)
 	return 0;
 }
 
+static int tipc_sockaddr_is_sane(struct sockaddr_tipc *addr)
+{
+	if (addr->family != AF_TIPC)
+		return 0;
+	if (addr->addrtype == TIPC_SERVICE_RANGE)
+		return (addr->addr.nameseq.lower <= addr->addr.nameseq.upper);
+	return (addr->addrtype == TIPC_SERVICE_ADDR ||
+		addr->addrtype == TIPC_SOCKET_ADDR);
+}
+
 /**
  * tipc_connect - establish a connection to another TIPC port
  * @sock: socket structure
@@ -2384,18 +2394,18 @@ static int tipc_connect(struct socket *sock, struct sockaddr *dest,
 		if (!tipc_sk_type_connectionless(sk))
 			res = -EINVAL;
 		goto exit;
-	} else if (dst->family != AF_TIPC) {
-		res = -EINVAL;
 	}
-	if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype != TIPC_ADDR_NAME)
+	if (!tipc_sockaddr_is_sane(dst)) {
 		res = -EINVAL;
-	if (res)
 		goto exit;
-
+	}
 	/* DGRAM/RDM connect(), just save the destaddr */
 	if (tipc_sk_type_connectionless(sk)) {
 		memcpy(&tsk->peer, dest, destlen);
 		goto exit;
+	} else if (dst->addrtype == TIPC_SERVICE_RANGE) {
+		res = -EINVAL;
+		goto exit;
 	}
 
 	previous = sk->sk_state;
-- 
2.1.4

Re: [tipc-discussion] [PATCH v2] tipc: allow service ranges to be connect()'ed on RDM/DGRAM

[Jon M. <jon...@er...>]

Don’t worry. I was just too bugged down with other issues the last days.
I’ll send it in today.

BR
///jon


From: Erik Hugne <eri...@gm...>
Sent: 15-Mar-19 14:53
To: tip...@li...; Jon Maloy <jon...@er...>; Xue, Ying <yin...@wi...>
Subject: Re: [PATCH v2] tipc: allow service ranges to be connect()'ed on RDM/DGRAM

 No love for this? :(
RDM connect is pretty handicapped right now

Re: [tipc-discussion] [PATCH v2] tipc: allow service ranges to be connect()'ed on RDM/DGRAM

[Erik H. <eri...@gm...>]

 No love for this? :(
RDM connect is pretty handicapped right now

[tipc-discussion] [PATCH v2] tipc: allow service ranges to be connect()'ed on RDM/DGRAM

[<eri...@gm...>]

From: Erik Hugne <eri...@gm...>

We move the check that prevents connecting service service ranges
to after the RDM/DGRAM check, and split out address sanitizing to
a separate function that also validates the service range.

Signed-off-by: Erik Hugne <eri...@gm...>
---
I was pondering to move all sanitize stuff (including length check) to 
one function and just call that before socket is locked, but the
AF_UNSPEC stuff made it complicated, so this just solves the problem
i had with connect()'ing a multicast address to an RDM socket.


 net/tipc/socket.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 3274ef625dba..195078128c0d 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2349,6 +2349,16 @@ static int tipc_wait_for_connect(struct socket *sock, long *timeo_p)
 	return 0;
 }
 
+static int tipc_sockaddr_is_sane(struct sockaddr_tipc *addr)
+{
+	if (addr->family != AF_TIPC)
+		return 0;
+	if (addr->addrtype == TIPC_SERVICE_RANGE)
+		return (addr->addr.nameseq.lower <= addr->addr.nameseq.upper);
+	return (addr->addrtype == TIPC_SERVICE_ADDR ||
+		addr->addrtype == TIPC_SOCKET_ADDR);
+}
+
 /**
  * tipc_connect - establish a connection to another TIPC port
  * @sock: socket structure
@@ -2384,18 +2394,18 @@ static int tipc_connect(struct socket *sock, struct sockaddr *dest,
 		if (!tipc_sk_type_connectionless(sk))
 			res = -EINVAL;
 		goto exit;
-	} else if (dst->family != AF_TIPC) {
-		res = -EINVAL;
 	}
-	if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype != TIPC_ADDR_NAME)
+	if (!tipc_sockaddr_is_sane(dst)) {
 		res = -EINVAL;
-	if (res)
 		goto exit;
-
+	}
 	/* DGRAM/RDM connect(), just save the destaddr */
 	if (tipc_sk_type_connectionless(sk)) {
 		memcpy(&tsk->peer, dest, destlen);
 		goto exit;
+	} else if (dst->addrtype == TIPC_SERVICE_RANGE) {
+		res = -EINVAL;
+		goto exit;
 	}
 
 	previous = sk->sk_state;
-- 
2.14.1

Re: [tipc-discussion] [PATCH net] tipc: allow multicast address to be associated for RDM/DGRAM sockets

[Jon M. <jon...@er...>]

I think we should make the sanity control here, even if it may be redundant. To first see connect() return ok, and then have a message rejected because is illogical and confusing to the user. Besides, this is not time critical.

///jon


From: Erik Hugne <eri...@gm...>
Sent: 9-Mar-19 03:42
To: tip...@li...; Jon Maloy <jon...@er...>; Xue, Ying <yin...@wi...>
Subject: Re: [PATCH net] tipc: allow multicast address to be associated for RDM/DGRAM sockets

OTOH, an invalid address will be caught on the first send(), where -EHOSTUNREACH will be returned.
Jon, what do you think?

Den fre 8 mars 2019 kl 21:16 skrev Erik Hugne <eri...@gm...<mailto:eri...@gm...>>:
I just realized that we may need to do some basic validation of TIPC_MULTICAST addresses.
With this patch, it will allow to connect() an address with lower > upper, which is bad.


Den fre 8 mars 2019 kl 20:09 skrev <eri...@gm...<mailto:eri...@gm...>>:
From: Erik Hugne <eri...@gm...<mailto:eri...@gm...>>

Perform the address type validation after we check if it's a
connectionless socket, allowing TIPC_MULTICAST addresses to be
associated with RDM/DGRAM sockets. This was broken in the commit
listed below.

Fixes: 23998835be98 ("tipc: improve address sanity check in tipc_connect()")
Signed-off-by: Erik Hugne <eri...@gm...<mailto:eri...@gm...>>
---
 net/tipc/socket.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index e482b342bfa8..0ad8e6585adf 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2387,16 +2387,15 @@ static int tipc_connect(struct socket *sock, struct sockaddr *dest,
        } else if (dst->family != AF_TIPC) {
                res = -EINVAL;
        }
-       if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype != TIPC_ADDR_NAME)
-               res = -EINVAL;
-       if (res)
-               goto exit;
-
        /* DGRAM/RDM connect(), just save the destaddr */
        if (tipc_sk_type_connectionless(sk)) {
                memcpy(&tsk->peer, dest, destlen);
                goto exit;
        }
+       if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype != TIPC_ADDR_NAME)
+               res = -EINVAL;
+       if (res)
+               goto exit;

        previous = sk->sk_state;

--
2.14.1

Re: [tipc-discussion] [PATCH net] tipc: allow multicast address to be associated for RDM/DGRAM sockets

[Erik H. <eri...@gm...>]

OTOH, an invalid address will be caught on the first send(), where
-EHOSTUNREACH will be returned.
Jon, what do you think?

Den fre 8 mars 2019 kl 21:16 skrev Erik Hugne <eri...@gm...>:

> I just realized that we may need to do some basic validation of
> TIPC_MULTICAST addresses.
> With this patch, it will allow to connect() an address with lower > upper,
> which is bad.
>
>
> Den fre 8 mars 2019 kl 20:09 skrev <eri...@gm...>:
>
>> From: Erik Hugne <eri...@gm...>
>>
>> Perform the address type validation after we check if it's a
>> connectionless socket, allowing TIPC_MULTICAST addresses to be
>> associated with RDM/DGRAM sockets. This was broken in the commit
>> listed below.
>>
>> Fixes: 23998835be98 ("tipc: improve address sanity check in
>> tipc_connect()")
>> Signed-off-by: Erik Hugne <eri...@gm...>
>> ---
>>  net/tipc/socket.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
>> index e482b342bfa8..0ad8e6585adf 100644
>> --- a/net/tipc/socket.c
>> +++ b/net/tipc/socket.c
>> @@ -2387,16 +2387,15 @@ static int tipc_connect(struct socket *sock,
>> struct sockaddr *dest,
>>         } else if (dst->family != AF_TIPC) {
>>                 res = -EINVAL;
>>         }
>> -       if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype !=
>> TIPC_ADDR_NAME)
>> -               res = -EINVAL;
>> -       if (res)
>> -               goto exit;
>> -
>>         /* DGRAM/RDM connect(), just save the destaddr */
>>         if (tipc_sk_type_connectionless(sk)) {
>>                 memcpy(&tsk->peer, dest, destlen);
>>                 goto exit;
>>         }
>> +       if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype !=
>> TIPC_ADDR_NAME)
>> +               res = -EINVAL;
>> +       if (res)
>> +               goto exit;
>>
>>         previous = sk->sk_state;
>>
>> --
>> 2.14.1
>>
>>

Re: [tipc-discussion] [PATCH net] tipc: allow multicast address to be associated for RDM/DGRAM sockets

[Erik H. <eri...@gm...>]

I just realized that we may need to do some basic validation of
TIPC_MULTICAST addresses.
With this patch, it will allow to connect() an address with lower > upper,
which is bad.


Den fre 8 mars 2019 kl 20:09 skrev <eri...@gm...>:

> From: Erik Hugne <eri...@gm...>
>
> Perform the address type validation after we check if it's a
> connectionless socket, allowing TIPC_MULTICAST addresses to be
> associated with RDM/DGRAM sockets. This was broken in the commit
> listed below.
>
> Fixes: 23998835be98 ("tipc: improve address sanity check in
> tipc_connect()")
> Signed-off-by: Erik Hugne <eri...@gm...>
> ---
>  net/tipc/socket.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index e482b342bfa8..0ad8e6585adf 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -2387,16 +2387,15 @@ static int tipc_connect(struct socket *sock,
> struct sockaddr *dest,
>         } else if (dst->family != AF_TIPC) {
>                 res = -EINVAL;
>         }
> -       if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype !=
> TIPC_ADDR_NAME)
> -               res = -EINVAL;
> -       if (res)
> -               goto exit;
> -
>         /* DGRAM/RDM connect(), just save the destaddr */
>         if (tipc_sk_type_connectionless(sk)) {
>                 memcpy(&tsk->peer, dest, destlen);
>                 goto exit;
>         }
> +       if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype !=
> TIPC_ADDR_NAME)
> +               res = -EINVAL;
> +       if (res)
> +               goto exit;
>
>         previous = sk->sk_state;
>
> --
> 2.14.1
>
>

[tipc-discussion] [PATCH net] tipc: allow multicast address to be associated for RDM/DGRAM sockets

[<eri...@gm...>]

From: Erik Hugne <eri...@gm...>

Perform the address type validation after we check if it's a
connectionless socket, allowing TIPC_MULTICAST addresses to be
associated with RDM/DGRAM sockets. This was broken in the commit 
listed below.

Fixes: 23998835be98 ("tipc: improve address sanity check in tipc_connect()")
Signed-off-by: Erik Hugne <eri...@gm...>
---
 net/tipc/socket.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index e482b342bfa8..0ad8e6585adf 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2387,16 +2387,15 @@ static int tipc_connect(struct socket *sock, struct sockaddr *dest,
 	} else if (dst->family != AF_TIPC) {
 		res = -EINVAL;
 	}
-	if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype != TIPC_ADDR_NAME)
-		res = -EINVAL;
-	if (res)
-		goto exit;
-
 	/* DGRAM/RDM connect(), just save the destaddr */
 	if (tipc_sk_type_connectionless(sk)) {
 		memcpy(&tsk->peer, dest, destlen);
 		goto exit;
 	}
+	if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype != TIPC_ADDR_NAME)
+		res = -EINVAL;
+	if (res)
+		goto exit;
 
 	previous = sk->sk_state;
 
-- 
2.14.1

Re: [tipc-discussion] [PATCH] tipc: allow multicast address to be associated for RDM/DGRAM sockets

[Erik H. <eri...@gm...>]

Seems to have been broken since:
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/net/tipc/socket.c?id=23998835be98a6842e5698fa1824f404c7de850d

I would say net.

Ill submit a new wit proper subject and "Fixes" tag.

//E

On Fri, 8 Mar 2019, 13:38 Jon Maloy, <jon...@er...> wrote:

> Looks ok with me. Was there something broken, or is it a new feature ?
> I.e., should I send it to net or net-next ?
>
> ///jon
>
>
> > -----Original Message-----
> > From: eri...@gm... <eri...@gm...>
> > Sent: 7-Mar-19 15:27
> > To: tip...@li...; Jon Maloy
> > <jon...@er...>; yin...@wi...
> > Cc: Erik Hugne <eri...@gm...>
> > Subject: [PATCH] tipc: allow multicast address to be associated for
> > RDM/DGRAM sockets
> >
> > From: Erik Hugne <eri...@gm...>
> >
> > Perform the address type validation after we check if it's a
> connectionless
> > socket, allowing TIPC_MULTICAST addresses to be associated with
> > RDM/DGRAM sockets.
> > ---
> >  net/tipc/socket.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/tipc/socket.c b/net/tipc/socket.c index
> > 3274ef625dba..dbfe12b07461 100644
> > --- a/net/tipc/socket.c
> > +++ b/net/tipc/socket.c
> > @@ -2387,16 +2387,15 @@ static int tipc_connect(struct socket *sock,
> struct
> > sockaddr *dest,
> >       } else if (dst->family != AF_TIPC) {
> >               res = -EINVAL;
> >       }
> > -     if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype !=
> > TIPC_ADDR_NAME)
> > -             res = -EINVAL;
> > -     if (res)
> > -             goto exit;
> > -
> >       /* DGRAM/RDM connect(), just save the destaddr */
> >       if (tipc_sk_type_connectionless(sk)) {
> >               memcpy(&tsk->peer, dest, destlen);
> >               goto exit;
> >       }
> > +     if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype !=
> > TIPC_ADDR_NAME)
> > +             res = -EINVAL;
> > +     if (res)
> > +             goto exit;
> >
> >       previous = sk->sk_state;
> >
> > --
> > 2.14.1
>
>

Re: [tipc-discussion] [PATCH] tipc: allow multicast address to be associated for RDM/DGRAM sockets

[Jon M. <jon...@er...>]

Looks ok with me. Was there something broken, or is it a new feature ?  I.e., should I send it to net or net-next ?

///jon


> -----Original Message-----
> From: eri...@gm... <eri...@gm...>
> Sent: 7-Mar-19 15:27
> To: tip...@li...; Jon Maloy
> <jon...@er...>; yin...@wi...
> Cc: Erik Hugne <eri...@gm...>
> Subject: [PATCH] tipc: allow multicast address to be associated for
> RDM/DGRAM sockets
> 
> From: Erik Hugne <eri...@gm...>
> 
> Perform the address type validation after we check if it's a connectionless
> socket, allowing TIPC_MULTICAST addresses to be associated with
> RDM/DGRAM sockets.
> ---
>  net/tipc/socket.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c index
> 3274ef625dba..dbfe12b07461 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -2387,16 +2387,15 @@ static int tipc_connect(struct socket *sock, struct
> sockaddr *dest,
>  	} else if (dst->family != AF_TIPC) {
>  		res = -EINVAL;
>  	}
> -	if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype !=
> TIPC_ADDR_NAME)
> -		res = -EINVAL;
> -	if (res)
> -		goto exit;
> -
>  	/* DGRAM/RDM connect(), just save the destaddr */
>  	if (tipc_sk_type_connectionless(sk)) {
>  		memcpy(&tsk->peer, dest, destlen);
>  		goto exit;
>  	}
> +	if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype !=
> TIPC_ADDR_NAME)
> +		res = -EINVAL;
> +	if (res)
> +		goto exit;
> 
>  	previous = sk->sk_state;
> 
> --
> 2.14.1

[tipc-discussion] [PATCH] tipc: allow multicast address to be associated for RDM/DGRAM sockets

[<eri...@gm...>]

From: Erik Hugne <eri...@gm...>

Perform the address type validation after we check if it's a
connectionless socket, allowing TIPC_MULTICAST addresses to be
associated with RDM/DGRAM sockets.
---
 net/tipc/socket.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 3274ef625dba..dbfe12b07461 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2387,16 +2387,15 @@ static int tipc_connect(struct socket *sock, struct sockaddr *dest,
 	} else if (dst->family != AF_TIPC) {
 		res = -EINVAL;
 	}
-	if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype != TIPC_ADDR_NAME)
-		res = -EINVAL;
-	if (res)
-		goto exit;
-
 	/* DGRAM/RDM connect(), just save the destaddr */
 	if (tipc_sk_type_connectionless(sk)) {
 		memcpy(&tsk->peer, dest, destlen);
 		goto exit;
 	}
+	if (dst->addrtype != TIPC_ADDR_ID && dst->addrtype != TIPC_ADDR_NAME)
+		res = -EINVAL;
+	if (res)
+		goto exit;
 
 	previous = sk->sk_state;
 
-- 
2.14.1

Re: [tipc-discussion] [net 1/1] tipc: fix RDM/DGRAM connect() regression

[David M. <da...@da...>]

From: Jon Maloy <jon...@er...>
Date: Mon, 4 Mar 2019 23:26:10 +0100

> From: Erik Hugne <eri...@gm...>
> 
> Fix regression bug introduced in
> commit 365ad353c256 ("tipc: reduce risk of user starvation during link
> congestion")
> 
> Only signal -EDESTADDRREQ for RDM/DGRAM if we don't have a cached
> sockaddr.
> 
> Signed-off-by: Erik Hugne <eri...@gm...>
> Signed-off-by: Jon Maloy <jon...@er...>

Please provide a proper Fixes: tag next time, I fixed it up for you
this time around.

Applied, thanks.

[tipc-discussion] [net 1/1] tipc: fix RDM/DGRAM connect() regression

[Jon M. <jon...@er...>]

From: Erik Hugne <eri...@gm...>

Fix regression bug introduced in
commit 365ad353c256 ("tipc: reduce risk of user starvation during link
congestion")

Only signal -EDESTADDRREQ for RDM/DGRAM if we don't have a cached
sockaddr.

Signed-off-by: Erik Hugne <eri...@gm...>
Signed-off-by: Jon Maloy <jon...@er...>
---
 net/tipc/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 70343ac..139694f 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1333,7 +1333,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 
 	if (unlikely(!dest)) {
 		dest = &tsk->peer;
-		if (!syn || dest->family != AF_TIPC)
+		if (!syn && dest->family != AF_TIPC)
 			return -EDESTADDRREQ;
 	}
 
-- 
2.1.4

[tipc-discussion] Thoughts/comments on bash redirect functionality?

[Erik H. <eri...@gm...>]

If you are interested in network redirection support for bash:
https://github.com/Hugne/bash/commit/master

Currently, it works in RDM/DGRAM mode only i want to keep the interface
simple and not involve any seqpacket/rdm switches in the file path
specification.

So currently, you can do something like this:

Write and read (a simple RDM echo server is running on 1000/123)
> # exec 3 <>/dev/tipc/1000/123
> # echo FOO >&3
> # cat <&3
FOO

Write in its simplest form
> # echo FOO > /dev/tipc/1000/123


I have not added any serving socket functionality, bash don't have that for
tcp/udp redir either.

Any thoughts or comments?

[tipc-discussion] [PATCH net] tipc: fix RDM/DGRAM connect() regression

[<eri...@gm...>]

From: Erik Hugne <eri...@gm...>

Fix regression bug introduced in
commit 365ad353c256 ("tipc: reduce risk of user starvation during link
congestion")

Only signal -EDESTADDRREQ for RDM/DGRAM if we don't have a cached
sockaddr.

Signed-off-by: Erik Hugne <eri...@gm...>
---
Jon/Ying, i have a very limited test setup, i hope you can review 
and ensure that the proposed change is safe.
I have tested it (on net-next) that it at least solves the problem
i had.

Once the fix is mainline, i will submit patches to bash for
shell network redirection.

This should be in net, because it's a bug that breaks connectionless
messaging in many ways.
Feel free to forward it there after your review/signoff.


 net/tipc/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index e482b342bfa8..3274ef625dba 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1333,7 +1333,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 
 	if (unlikely(!dest)) {
 		dest = &tsk->peer;
-		if (!syn || dest->family != AF_TIPC)
+		if (!syn && dest->family != AF_TIPC)
 			return -EDESTADDRREQ;
 	}
 
-- 
2.14.1

Re: [tipc-discussion] RDM connect error

[Erik H. <eri...@gm...>]

Yes, this is a regression bug.. tested on my ubuntu vm with 4.13 kernel
It seems that it's still in net-next,

https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/tree/net/tipc/socket.c#n1334

syn is always false for RDM, leading to this check to fail with EDESTADDRREQ


In older kernels there was another bug where m->msg_namelen was checked
(even if we used a cached sockaddr), but that seems to have been fixed



Den lör 2 mars 2019 kl 19:57 skrev Erik Hugne <eri...@gm...>:

> It was added in this one, sorry
>
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/net/tipc/socket.c?id=f2f8036e391eb82ee78764483f869f2feafb5da8
>
> Den lör 2 mars 2019 kl 19:21 skrev Erik Hugne <eri...@gm...>:
>
>> Hi, i dont seem to be able to post to the list for some reason.. :/
>>
>>
>> I'm doing some TIPC related work on the GNU bash project, and got stuck
>> when i tried to set up a RDM socket and associate a sockaddr with it (as
>> was introduced with this commit)
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/net/tipc/socket.c?id=66bc1e8d5d1d156b1e85d8c6925225ad8cbdf523
>>
>>
>> sendmsg() return -EDESTADDRREQ..
>>
>> I dont have any bash code to share yet, but you can reproduce the with
>> the TLS demo:
>> https://github.com/Hugne/tls_tipc
>>
>