tipc-discussion

[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] [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

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
>
>

[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 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
>
>

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

[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