* [PATCH net 0/2] fix memory leak for sctp_do_bind
@ 2019-09-10 7:13 Mao Wenan
2019-09-10 7:13 ` [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local Mao Wenan
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Mao Wenan @ 2019-09-10 7:13 UTC (permalink / raw)
To: vyasevich, nhorman, marcelo.leitner, davem
Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan
First patch is to do cleanup, remove redundant assignment,
second patch is to fix memory leak for sctp_do_bind if failed
to bind address.
Mao Wenan (2):
sctp: remove redundant assignment when call sctp_get_port_local
sctp: destroy bucket if failed to bind addr
net/sctp/socket.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local 2019-09-10 7:13 [PATCH net 0/2] fix memory leak for sctp_do_bind Mao Wenan @ 2019-09-10 7:13 ` Mao Wenan 2019-09-10 18:57 ` Dan Carpenter 2019-09-10 7:13 ` [PATCH net 2/2] sctp: destroy bucket if failed to bind addr Mao Wenan 2019-09-10 7:16 ` [PATCH net 0/2] fix memory leak for sctp_do_bind Neil Horman 2 siblings, 1 reply; 19+ messages in thread From: Mao Wenan @ 2019-09-10 7:13 UTC (permalink / raw) To: vyasevich, nhorman, marcelo.leitner, davem Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan There are more parentheses in if clause when call sctp_get_port_local in sctp_do_bind, and redundant assignment to 'ret'. This patch is to do cleanup. Signed-off-by: Mao Wenan <maowenan@huawei.com> --- net/sctp/socket.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 9d1f83b10c0a..766b68b55ebe 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) * detection. */ addr->v4.sin_port = htons(snum); - if ((ret = sctp_get_port_local(sk, addr))) { + if (sctp_get_port_local(sk, addr)) return -EADDRINUSE; - } /* Refresh ephemeral port. */ if (!bp->port) -- 2.20.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local 2019-09-10 7:13 ` [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local Mao Wenan @ 2019-09-10 18:57 ` Dan Carpenter 2019-09-10 19:22 ` Dan Carpenter 0 siblings, 1 reply; 19+ messages in thread From: Dan Carpenter @ 2019-09-10 18:57 UTC (permalink / raw) To: Mao Wenan Cc: vyasevich, nhorman, marcelo.leitner, davem, linux-sctp, netdev, linux-kernel, kernel-janitors On Tue, Sep 10, 2019 at 03:13:42PM +0800, Mao Wenan wrote: > There are more parentheses in if clause when call sctp_get_port_local > in sctp_do_bind, and redundant assignment to 'ret'. This patch is to > do cleanup. > > Signed-off-by: Mao Wenan <maowenan@huawei.com> > --- > net/sctp/socket.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index 9d1f83b10c0a..766b68b55ebe 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) > * detection. > */ > addr->v4.sin_port = htons(snum); > - if ((ret = sctp_get_port_local(sk, addr))) { > + if (sctp_get_port_local(sk, addr)) > return -EADDRINUSE; sctp_get_port_local() returns a long which is either 0,1 or a pointer casted to long. It's not documented what it means and neither of the callers use the return since commit 62208f12451f ("net: sctp: simplify sctp_get_port"). Probably it should just return a bool? regards, dan carpenter ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local 2019-09-10 18:57 ` Dan Carpenter @ 2019-09-10 19:22 ` Dan Carpenter 2019-09-11 1:30 ` maowenan 0 siblings, 1 reply; 19+ messages in thread From: Dan Carpenter @ 2019-09-10 19:22 UTC (permalink / raw) To: Mao Wenan Cc: vyasevich, nhorman, marcelo.leitner, davem, linux-sctp, netdev, linux-kernel, kernel-janitors On Tue, Sep 10, 2019 at 09:57:10PM +0300, Dan Carpenter wrote: > On Tue, Sep 10, 2019 at 03:13:42PM +0800, Mao Wenan wrote: > > There are more parentheses in if clause when call sctp_get_port_local > > in sctp_do_bind, and redundant assignment to 'ret'. This patch is to > > do cleanup. > > > > Signed-off-by: Mao Wenan <maowenan@huawei.com> > > --- > > net/sctp/socket.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > index 9d1f83b10c0a..766b68b55ebe 100644 > > --- a/net/sctp/socket.c > > +++ b/net/sctp/socket.c > > @@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) > > * detection. > > */ > > addr->v4.sin_port = htons(snum); > > - if ((ret = sctp_get_port_local(sk, addr))) { > > + if (sctp_get_port_local(sk, addr)) > > return -EADDRINUSE; > > sctp_get_port_local() returns a long which is either 0,1 or a pointer > casted to long. It's not documented what it means and neither of the > callers use the return since commit 62208f12451f ("net: sctp: simplify > sctp_get_port"). Actually it was commit 4e54064e0a13 ("sctp: Allow only 1 listening socket with SO_REUSEADDR") from 11 years ago. That patch fixed a bug, because before the code assumed that a pointer casted to an int was the same as a pointer casted to a long. regards, dan carpenter ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local 2019-09-10 19:22 ` Dan Carpenter @ 2019-09-11 1:30 ` maowenan 2019-09-11 8:30 ` Dan Carpenter 0 siblings, 1 reply; 19+ messages in thread From: maowenan @ 2019-09-11 1:30 UTC (permalink / raw) To: Dan Carpenter Cc: vyasevich, nhorman, marcelo.leitner, davem, linux-sctp, netdev, linux-kernel, kernel-janitors On 2019/9/11 3:22, Dan Carpenter wrote: > On Tue, Sep 10, 2019 at 09:57:10PM +0300, Dan Carpenter wrote: >> On Tue, Sep 10, 2019 at 03:13:42PM +0800, Mao Wenan wrote: >>> There are more parentheses in if clause when call sctp_get_port_local >>> in sctp_do_bind, and redundant assignment to 'ret'. This patch is to >>> do cleanup. >>> >>> Signed-off-by: Mao Wenan <maowenan@huawei.com> >>> --- >>> net/sctp/socket.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c >>> index 9d1f83b10c0a..766b68b55ebe 100644 >>> --- a/net/sctp/socket.c >>> +++ b/net/sctp/socket.c >>> @@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) >>> * detection. >>> */ >>> addr->v4.sin_port = htons(snum); >>> - if ((ret = sctp_get_port_local(sk, addr))) { >>> + if (sctp_get_port_local(sk, addr)) >>> return -EADDRINUSE; >> >> sctp_get_port_local() returns a long which is either 0,1 or a pointer >> casted to long. It's not documented what it means and neither of the >> callers use the return since commit 62208f12451f ("net: sctp: simplify >> sctp_get_port"). > > Actually it was commit 4e54064e0a13 ("sctp: Allow only 1 listening > socket with SO_REUSEADDR") from 11 years ago. That patch fixed a bug, > because before the code assumed that a pointer casted to an int was the > same as a pointer casted to a long. commit 4e54064e0a13 treated non-zero return value as unexpected, so the current cleanup is ok? > > regards, > dan carpenter > > > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local 2019-09-11 1:30 ` maowenan @ 2019-09-11 8:30 ` Dan Carpenter 2019-09-11 14:30 ` Marcelo Ricardo Leitner 0 siblings, 1 reply; 19+ messages in thread From: Dan Carpenter @ 2019-09-11 8:30 UTC (permalink / raw) To: maowenan Cc: vyasevich, nhorman, marcelo.leitner, davem, linux-sctp, netdev, linux-kernel, kernel-janitors On Wed, Sep 11, 2019 at 09:30:47AM +0800, maowenan wrote: > > > On 2019/9/11 3:22, Dan Carpenter wrote: > > On Tue, Sep 10, 2019 at 09:57:10PM +0300, Dan Carpenter wrote: > >> On Tue, Sep 10, 2019 at 03:13:42PM +0800, Mao Wenan wrote: > >>> There are more parentheses in if clause when call sctp_get_port_local > >>> in sctp_do_bind, and redundant assignment to 'ret'. This patch is to > >>> do cleanup. > >>> > >>> Signed-off-by: Mao Wenan <maowenan@huawei.com> > >>> --- > >>> net/sctp/socket.c | 3 +-- > >>> 1 file changed, 1 insertion(+), 2 deletions(-) > >>> > >>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c > >>> index 9d1f83b10c0a..766b68b55ebe 100644 > >>> --- a/net/sctp/socket.c > >>> +++ b/net/sctp/socket.c > >>> @@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) > >>> * detection. > >>> */ > >>> addr->v4.sin_port = htons(snum); > >>> - if ((ret = sctp_get_port_local(sk, addr))) { > >>> + if (sctp_get_port_local(sk, addr)) > >>> return -EADDRINUSE; > >> > >> sctp_get_port_local() returns a long which is either 0,1 or a pointer > >> casted to long. It's not documented what it means and neither of the > >> callers use the return since commit 62208f12451f ("net: sctp: simplify > >> sctp_get_port"). > > > > Actually it was commit 4e54064e0a13 ("sctp: Allow only 1 listening > > socket with SO_REUSEADDR") from 11 years ago. That patch fixed a bug, > > because before the code assumed that a pointer casted to an int was the > > same as a pointer casted to a long. > > commit 4e54064e0a13 treated non-zero return value as unexpected, so the current > cleanup is ok? Yeah. It's fine, I was just confused why we weren't preserving the error code and then I saw that we didn't return errors at all and got confused. regards, dan carpenter ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local 2019-09-11 8:30 ` Dan Carpenter @ 2019-09-11 14:30 ` Marcelo Ricardo Leitner 2019-09-11 14:39 ` Marcelo Ricardo Leitner 0 siblings, 1 reply; 19+ messages in thread From: Marcelo Ricardo Leitner @ 2019-09-11 14:30 UTC (permalink / raw) To: Dan Carpenter Cc: maowenan, vyasevich, nhorman, davem, linux-sctp, netdev, linux-kernel, kernel-janitors On Wed, Sep 11, 2019 at 11:30:38AM +0300, Dan Carpenter wrote: > On Wed, Sep 11, 2019 at 09:30:47AM +0800, maowenan wrote: > > > > > > On 2019/9/11 3:22, Dan Carpenter wrote: > > > On Tue, Sep 10, 2019 at 09:57:10PM +0300, Dan Carpenter wrote: > > >> On Tue, Sep 10, 2019 at 03:13:42PM +0800, Mao Wenan wrote: > > >>> There are more parentheses in if clause when call sctp_get_port_local > > >>> in sctp_do_bind, and redundant assignment to 'ret'. This patch is to > > >>> do cleanup. > > >>> > > >>> Signed-off-by: Mao Wenan <maowenan@huawei.com> > > >>> --- > > >>> net/sctp/socket.c | 3 +-- > > >>> 1 file changed, 1 insertion(+), 2 deletions(-) > > >>> > > >>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > >>> index 9d1f83b10c0a..766b68b55ebe 100644 > > >>> --- a/net/sctp/socket.c > > >>> +++ b/net/sctp/socket.c > > >>> @@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) > > >>> * detection. > > >>> */ > > >>> addr->v4.sin_port = htons(snum); > > >>> - if ((ret = sctp_get_port_local(sk, addr))) { > > >>> + if (sctp_get_port_local(sk, addr)) > > >>> return -EADDRINUSE; > > >> > > >> sctp_get_port_local() returns a long which is either 0,1 or a pointer > > >> casted to long. It's not documented what it means and neither of the > > >> callers use the return since commit 62208f12451f ("net: sctp: simplify > > >> sctp_get_port"). > > > > > > Actually it was commit 4e54064e0a13 ("sctp: Allow only 1 listening > > > socket with SO_REUSEADDR") from 11 years ago. That patch fixed a bug, > > > because before the code assumed that a pointer casted to an int was the > > > same as a pointer casted to a long. > > > > commit 4e54064e0a13 treated non-zero return value as unexpected, so the current > > cleanup is ok? > > Yeah. It's fine, I was just confused why we weren't preserving the > error code and then I saw that we didn't return errors at all and got > confused. But please lets seize the moment and do the change Dean suggested. This was the last place saving this return value somewhere. It makes sense to cleanup sctp_get_port_local() now and remove that masked pointer return. Then you may also cleanup: socket.c: return !!sctp_get_port_local(sk, &addr); as it will be a direct map. Marcelo ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local 2019-09-11 14:30 ` Marcelo Ricardo Leitner @ 2019-09-11 14:39 ` Marcelo Ricardo Leitner 2019-09-12 2:05 ` maowenan 0 siblings, 1 reply; 19+ messages in thread From: Marcelo Ricardo Leitner @ 2019-09-11 14:39 UTC (permalink / raw) To: Dan Carpenter Cc: maowenan, vyasevich, nhorman, davem, linux-sctp, netdev, linux-kernel, kernel-janitors On Wed, Sep 11, 2019 at 11:30:08AM -0300, Marcelo Ricardo Leitner wrote: > On Wed, Sep 11, 2019 at 11:30:38AM +0300, Dan Carpenter wrote: > > On Wed, Sep 11, 2019 at 09:30:47AM +0800, maowenan wrote: > > > > > > > > > On 2019/9/11 3:22, Dan Carpenter wrote: > > > > On Tue, Sep 10, 2019 at 09:57:10PM +0300, Dan Carpenter wrote: > > > >> On Tue, Sep 10, 2019 at 03:13:42PM +0800, Mao Wenan wrote: > > > >>> There are more parentheses in if clause when call sctp_get_port_local > > > >>> in sctp_do_bind, and redundant assignment to 'ret'. This patch is to > > > >>> do cleanup. > > > >>> > > > >>> Signed-off-by: Mao Wenan <maowenan@huawei.com> > > > >>> --- > > > >>> net/sctp/socket.c | 3 +-- > > > >>> 1 file changed, 1 insertion(+), 2 deletions(-) > > > >>> > > > >>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > >>> index 9d1f83b10c0a..766b68b55ebe 100644 > > > >>> --- a/net/sctp/socket.c > > > >>> +++ b/net/sctp/socket.c > > > >>> @@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) > > > >>> * detection. > > > >>> */ > > > >>> addr->v4.sin_port = htons(snum); > > > >>> - if ((ret = sctp_get_port_local(sk, addr))) { > > > >>> + if (sctp_get_port_local(sk, addr)) > > > >>> return -EADDRINUSE; > > > >> > > > >> sctp_get_port_local() returns a long which is either 0,1 or a pointer > > > >> casted to long. It's not documented what it means and neither of the > > > >> callers use the return since commit 62208f12451f ("net: sctp: simplify > > > >> sctp_get_port"). > > > > > > > > Actually it was commit 4e54064e0a13 ("sctp: Allow only 1 listening > > > > socket with SO_REUSEADDR") from 11 years ago. That patch fixed a bug, > > > > because before the code assumed that a pointer casted to an int was the > > > > same as a pointer casted to a long. > > > > > > commit 4e54064e0a13 treated non-zero return value as unexpected, so the current > > > cleanup is ok? > > > > Yeah. It's fine, I was just confused why we weren't preserving the > > error code and then I saw that we didn't return errors at all and got > > confused. > > But please lets seize the moment and do the change Dean suggested. *Dan*, sorry. > This was the last place saving this return value somewhere. It makes > sense to cleanup sctp_get_port_local() now and remove that masked > pointer return. > > Then you may also cleanup: > socket.c: return !!sctp_get_port_local(sk, &addr); > as it will be a direct map. > > Marcelo > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local 2019-09-11 14:39 ` Marcelo Ricardo Leitner @ 2019-09-12 2:05 ` maowenan 2019-09-12 4:02 ` [PATCH v2 net 0/3] fix memory leak for sctp_do_bind Mao Wenan 0 siblings, 1 reply; 19+ messages in thread From: maowenan @ 2019-09-12 2:05 UTC (permalink / raw) To: Marcelo Ricardo Leitner, Dan Carpenter Cc: vyasevich, nhorman, davem, linux-sctp, netdev, linux-kernel, kernel-janitors On 2019/9/11 22:39, Marcelo Ricardo Leitner wrote: > On Wed, Sep 11, 2019 at 11:30:08AM -0300, Marcelo Ricardo Leitner wrote: >> On Wed, Sep 11, 2019 at 11:30:38AM +0300, Dan Carpenter wrote: >>> On Wed, Sep 11, 2019 at 09:30:47AM +0800, maowenan wrote: >>>> >>>> >>>> On 2019/9/11 3:22, Dan Carpenter wrote: >>>>> On Tue, Sep 10, 2019 at 09:57:10PM +0300, Dan Carpenter wrote: >>>>>> On Tue, Sep 10, 2019 at 03:13:42PM +0800, Mao Wenan wrote: >>>>>>> There are more parentheses in if clause when call sctp_get_port_local >>>>>>> in sctp_do_bind, and redundant assignment to 'ret'. This patch is to >>>>>>> do cleanup. >>>>>>> >>>>>>> Signed-off-by: Mao Wenan <maowenan@huawei.com> >>>>>>> --- >>>>>>> net/sctp/socket.c | 3 +-- >>>>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c >>>>>>> index 9d1f83b10c0a..766b68b55ebe 100644 >>>>>>> --- a/net/sctp/socket.c >>>>>>> +++ b/net/sctp/socket.c >>>>>>> @@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) >>>>>>> * detection. >>>>>>> */ >>>>>>> addr->v4.sin_port = htons(snum); >>>>>>> - if ((ret = sctp_get_port_local(sk, addr))) { >>>>>>> + if (sctp_get_port_local(sk, addr)) >>>>>>> return -EADDRINUSE; >>>>>> >>>>>> sctp_get_port_local() returns a long which is either 0,1 or a pointer >>>>>> casted to long. It's not documented what it means and neither of the >>>>>> callers use the return since commit 62208f12451f ("net: sctp: simplify >>>>>> sctp_get_port"). >>>>> >>>>> Actually it was commit 4e54064e0a13 ("sctp: Allow only 1 listening >>>>> socket with SO_REUSEADDR") from 11 years ago. That patch fixed a bug, >>>>> because before the code assumed that a pointer casted to an int was the >>>>> same as a pointer casted to a long. >>>> >>>> commit 4e54064e0a13 treated non-zero return value as unexpected, so the current >>>> cleanup is ok? >>> >>> Yeah. It's fine, I was just confused why we weren't preserving the >>> error code and then I saw that we didn't return errors at all and got >>> confused. >> >> But please lets seize the moment and do the change Dean suggested. > > *Dan*, sorry. > >> This was the last place saving this return value somewhere. It makes >> sense to cleanup sctp_get_port_local() now and remove that masked >> pointer return. >> >> Then you may also cleanup: >> socket.c: return !!sctp_get_port_local(sk, &addr); >> as it will be a direct map. Thanks Marcelo, shall I post a new individual patch for cleanup as your suggest? >> >> Marcelo >> > > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 net 0/3] fix memory leak for sctp_do_bind 2019-09-12 2:05 ` maowenan @ 2019-09-12 4:02 ` Mao Wenan 2019-09-12 4:02 ` [PATCH v2 net 1/3] sctp: change return type of sctp_get_port_local Mao Wenan ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Mao Wenan @ 2019-09-12 4:02 UTC (permalink / raw) To: vyasevich, nhorman, marcelo.leitner, davem Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan First two patches are to do cleanup, remove redundant assignment, and change return type of sctp_get_port_local. Third patch is to fix memory leak for sctp_do_bind if failed to bind address. --- v2: add one patch to change return type of sctp_get_port_local. --- Mao Wenan (3): sctp: change return type of sctp_get_port_local sctp: remove redundant assignment when call sctp_get_port_local sctp: destroy bucket if failed to bind addr net/sctp/socket.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 net 1/3] sctp: change return type of sctp_get_port_local 2019-09-12 4:02 ` [PATCH v2 net 0/3] fix memory leak for sctp_do_bind Mao Wenan @ 2019-09-12 4:02 ` Mao Wenan 2019-09-12 14:51 ` Marcelo Ricardo Leitner 2019-09-12 4:02 ` [PATCH v2 net 2/3] sctp: remove redundant assignment when call sctp_get_port_local Mao Wenan ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Mao Wenan @ 2019-09-12 4:02 UTC (permalink / raw) To: vyasevich, nhorman, marcelo.leitner, davem Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan Currently sctp_get_port_local() returns a long which is either 0,1 or a pointer casted to long. It's neither of the callers use the return value since commit 62208f12451f ("net: sctp: simplify sctp_get_port"). Now two callers are sctp_get_port and sctp_do_bind, they actually assumend a casted to an int was the same as a pointer casted to a long, and they don't save the return value just check whether it is zero or non-zero, so it would better change return type from long to int for sctp_get_port_local. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Mao Wenan <maowenan@huawei.com> --- net/sctp/socket.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 9d1f83b10c0a..5e1934c48709 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -309,7 +309,7 @@ static int sctp_bind(struct sock *sk, struct sockaddr *addr, int addr_len) return retval; } -static long sctp_get_port_local(struct sock *, union sctp_addr *); +static int sctp_get_port_local(struct sock *, union sctp_addr *); /* Verify this is a valid sockaddr. */ static struct sctp_af *sctp_sockaddr_af(struct sctp_sock *opt, @@ -7998,7 +7998,7 @@ static void sctp_unhash(struct sock *sk) static struct sctp_bind_bucket *sctp_bucket_create( struct sctp_bind_hashbucket *head, struct net *, unsigned short snum); -static long sctp_get_port_local(struct sock *sk, union sctp_addr *addr) +static int sctp_get_port_local(struct sock *sk, union sctp_addr *addr) { struct sctp_sock *sp = sctp_sk(sk); bool reuse = (sk->sk_reuse || sp->reuse); @@ -8108,7 +8108,7 @@ static long sctp_get_port_local(struct sock *sk, union sctp_addr *addr) if (sctp_bind_addr_conflict(&ep2->base.bind_addr, addr, sp2, sp)) { - ret = (long)sk2; + ret = 1; goto fail_unlock; } } @@ -8180,7 +8180,7 @@ static int sctp_get_port(struct sock *sk, unsigned short snum) addr.v4.sin_port = htons(snum); /* Note: sk->sk_num gets filled in if ephemeral port request. */ - return !!sctp_get_port_local(sk, &addr); + return sctp_get_port_local(sk, &addr); } /* -- 2.20.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 net 1/3] sctp: change return type of sctp_get_port_local 2019-09-12 4:02 ` [PATCH v2 net 1/3] sctp: change return type of sctp_get_port_local Mao Wenan @ 2019-09-12 14:51 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 19+ messages in thread From: Marcelo Ricardo Leitner @ 2019-09-12 14:51 UTC (permalink / raw) To: Mao Wenan Cc: vyasevich, nhorman, davem, linux-sctp, netdev, linux-kernel, kernel-janitors On Thu, Sep 12, 2019 at 12:02:17PM +0800, Mao Wenan wrote: > Currently sctp_get_port_local() returns a long > which is either 0,1 or a pointer casted to long. > It's neither of the callers use the return value since > commit 62208f12451f ("net: sctp: simplify sctp_get_port"). > Now two callers are sctp_get_port and sctp_do_bind, > they actually assumend a casted to an int was the same as > a pointer casted to a long, and they don't save the return > value just check whether it is zero or non-zero, so > it would better change return type from long to int for > sctp_get_port_local. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") This Fixes tag is not needed. It's just a cleanup. Maybe Dave can remove it for us. Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> Thanks. > Signed-off-by: Mao Wenan <maowenan@huawei.com> > --- > net/sctp/socket.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index 9d1f83b10c0a..5e1934c48709 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -309,7 +309,7 @@ static int sctp_bind(struct sock *sk, struct sockaddr *addr, int addr_len) > return retval; > } > > -static long sctp_get_port_local(struct sock *, union sctp_addr *); > +static int sctp_get_port_local(struct sock *, union sctp_addr *); > > /* Verify this is a valid sockaddr. */ > static struct sctp_af *sctp_sockaddr_af(struct sctp_sock *opt, > @@ -7998,7 +7998,7 @@ static void sctp_unhash(struct sock *sk) > static struct sctp_bind_bucket *sctp_bucket_create( > struct sctp_bind_hashbucket *head, struct net *, unsigned short snum); > > -static long sctp_get_port_local(struct sock *sk, union sctp_addr *addr) > +static int sctp_get_port_local(struct sock *sk, union sctp_addr *addr) > { > struct sctp_sock *sp = sctp_sk(sk); > bool reuse = (sk->sk_reuse || sp->reuse); > @@ -8108,7 +8108,7 @@ static long sctp_get_port_local(struct sock *sk, union sctp_addr *addr) > > if (sctp_bind_addr_conflict(&ep2->base.bind_addr, > addr, sp2, sp)) { > - ret = (long)sk2; > + ret = 1; > goto fail_unlock; > } > } > @@ -8180,7 +8180,7 @@ static int sctp_get_port(struct sock *sk, unsigned short snum) > addr.v4.sin_port = htons(snum); > > /* Note: sk->sk_num gets filled in if ephemeral port request. */ > - return !!sctp_get_port_local(sk, &addr); > + return sctp_get_port_local(sk, &addr); > } > > /* > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 net 2/3] sctp: remove redundant assignment when call sctp_get_port_local 2019-09-12 4:02 ` [PATCH v2 net 0/3] fix memory leak for sctp_do_bind Mao Wenan 2019-09-12 4:02 ` [PATCH v2 net 1/3] sctp: change return type of sctp_get_port_local Mao Wenan @ 2019-09-12 4:02 ` Mao Wenan 2019-09-12 14:52 ` Marcelo Ricardo Leitner 2019-09-12 4:02 ` [PATCH v2 net 3/3] sctp: destroy bucket if failed to bind addr Mao Wenan 2019-09-13 20:06 ` [PATCH v2 net 0/3] fix memory leak for sctp_do_bind David Miller 3 siblings, 1 reply; 19+ messages in thread From: Mao Wenan @ 2019-09-12 4:02 UTC (permalink / raw) To: vyasevich, nhorman, marcelo.leitner, davem Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan There are more parentheses in if clause when call sctp_get_port_local in sctp_do_bind, and redundant assignment to 'ret'. This patch is to do cleanup. Signed-off-by: Mao Wenan <maowenan@huawei.com> Acked-by: Neil Horman <nhorman@tuxdriver.com> --- net/sctp/socket.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 5e1934c48709..2f810078c91d 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -399,9 +399,8 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) * detection. */ addr->v4.sin_port = htons(snum); - if ((ret = sctp_get_port_local(sk, addr))) { + if (sctp_get_port_local(sk, addr)) return -EADDRINUSE; - } /* Refresh ephemeral port. */ if (!bp->port) -- 2.20.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 net 2/3] sctp: remove redundant assignment when call sctp_get_port_local 2019-09-12 4:02 ` [PATCH v2 net 2/3] sctp: remove redundant assignment when call sctp_get_port_local Mao Wenan @ 2019-09-12 14:52 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 19+ messages in thread From: Marcelo Ricardo Leitner @ 2019-09-12 14:52 UTC (permalink / raw) To: Mao Wenan Cc: vyasevich, nhorman, davem, linux-sctp, netdev, linux-kernel, kernel-janitors On Thu, Sep 12, 2019 at 12:02:18PM +0800, Mao Wenan wrote: > There are more parentheses in if clause when call sctp_get_port_local > in sctp_do_bind, and redundant assignment to 'ret'. This patch is to > do cleanup. > > Signed-off-by: Mao Wenan <maowenan@huawei.com> > Acked-by: Neil Horman <nhorman@tuxdriver.com> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 net 3/3] sctp: destroy bucket if failed to bind addr 2019-09-12 4:02 ` [PATCH v2 net 0/3] fix memory leak for sctp_do_bind Mao Wenan 2019-09-12 4:02 ` [PATCH v2 net 1/3] sctp: change return type of sctp_get_port_local Mao Wenan 2019-09-12 4:02 ` [PATCH v2 net 2/3] sctp: remove redundant assignment when call sctp_get_port_local Mao Wenan @ 2019-09-12 4:02 ` Mao Wenan 2019-09-12 14:52 ` Marcelo Ricardo Leitner 2019-09-13 20:06 ` [PATCH v2 net 0/3] fix memory leak for sctp_do_bind David Miller 3 siblings, 1 reply; 19+ messages in thread From: Mao Wenan @ 2019-09-12 4:02 UTC (permalink / raw) To: vyasevich, nhorman, marcelo.leitner, davem Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan, Hulk Robot There is one memory leak bug report: BUG: memory leak unreferenced object 0xffff8881dc4c5ec0 (size 40): comm "syz-executor.0", pid 5673, jiffies 4298198457 (age 27.578s) hex dump (first 32 bytes): 02 00 00 00 81 88 ff ff 00 00 00 00 00 00 00 00 ................ f8 63 3d c1 81 88 ff ff 00 00 00 00 00 00 00 00 .c=............. backtrace: [<0000000072006339>] sctp_get_port_local+0x2a1/0xa00 [sctp] [<00000000c7b379ec>] sctp_do_bind+0x176/0x2c0 [sctp] [<000000005be274a2>] sctp_bind+0x5a/0x80 [sctp] [<00000000b66b4044>] inet6_bind+0x59/0xd0 [ipv6] [<00000000c68c7f42>] __sys_bind+0x120/0x1f0 net/socket.c:1647 [<000000004513635b>] __do_sys_bind net/socket.c:1658 [inline] [<000000004513635b>] __se_sys_bind net/socket.c:1656 [inline] [<000000004513635b>] __x64_sys_bind+0x3e/0x50 net/socket.c:1656 [<0000000061f2501e>] do_syscall_64+0x72/0x2e0 arch/x86/entry/common.c:296 [<0000000003d1e05e>] entry_SYSCALL_64_after_hwframe+0x49/0xbe This is because in sctp_do_bind, if sctp_get_port_local is to create hash bucket successfully, and sctp_add_bind_addr failed to bind address, e.g return -ENOMEM, so memory leak found, it needs to destroy allocated bucket. Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Mao Wenan <maowenan@huawei.com> Acked-by: Neil Horman <nhorman@tuxdriver.com> --- net/sctp/socket.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 2f810078c91d..69ec3b796197 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -412,11 +412,13 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len, SCTP_ADDR_SRC, GFP_ATOMIC); - /* Copy back into socket for getsockname() use. */ - if (!ret) { - inet_sk(sk)->inet_sport = htons(inet_sk(sk)->inet_num); - sp->pf->to_sk_saddr(addr, sk); + if (ret) { + sctp_put_port(sk); + return ret; } + /* Copy back into socket for getsockname() use. */ + inet_sk(sk)->inet_sport = htons(inet_sk(sk)->inet_num); + sp->pf->to_sk_saddr(addr, sk); return ret; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 net 3/3] sctp: destroy bucket if failed to bind addr 2019-09-12 4:02 ` [PATCH v2 net 3/3] sctp: destroy bucket if failed to bind addr Mao Wenan @ 2019-09-12 14:52 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 19+ messages in thread From: Marcelo Ricardo Leitner @ 2019-09-12 14:52 UTC (permalink / raw) To: Mao Wenan Cc: vyasevich, nhorman, davem, linux-sctp, netdev, linux-kernel, kernel-janitors, Hulk Robot On Thu, Sep 12, 2019 at 12:02:19PM +0800, Mao Wenan wrote: > There is one memory leak bug report: > BUG: memory leak > unreferenced object 0xffff8881dc4c5ec0 (size 40): > comm "syz-executor.0", pid 5673, jiffies 4298198457 (age 27.578s) > hex dump (first 32 bytes): > 02 00 00 00 81 88 ff ff 00 00 00 00 00 00 00 00 ................ > f8 63 3d c1 81 88 ff ff 00 00 00 00 00 00 00 00 .c=............. > backtrace: > [<0000000072006339>] sctp_get_port_local+0x2a1/0xa00 [sctp] > [<00000000c7b379ec>] sctp_do_bind+0x176/0x2c0 [sctp] > [<000000005be274a2>] sctp_bind+0x5a/0x80 [sctp] > [<00000000b66b4044>] inet6_bind+0x59/0xd0 [ipv6] > [<00000000c68c7f42>] __sys_bind+0x120/0x1f0 net/socket.c:1647 > [<000000004513635b>] __do_sys_bind net/socket.c:1658 [inline] > [<000000004513635b>] __se_sys_bind net/socket.c:1656 [inline] > [<000000004513635b>] __x64_sys_bind+0x3e/0x50 net/socket.c:1656 > [<0000000061f2501e>] do_syscall_64+0x72/0x2e0 arch/x86/entry/common.c:296 > [<0000000003d1e05e>] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > This is because in sctp_do_bind, if sctp_get_port_local is to > create hash bucket successfully, and sctp_add_bind_addr failed > to bind address, e.g return -ENOMEM, so memory leak found, it > needs to destroy allocated bucket. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Mao Wenan <maowenan@huawei.com> > Acked-by: Neil Horman <nhorman@tuxdriver.com> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 net 0/3] fix memory leak for sctp_do_bind 2019-09-12 4:02 ` [PATCH v2 net 0/3] fix memory leak for sctp_do_bind Mao Wenan ` (2 preceding siblings ...) 2019-09-12 4:02 ` [PATCH v2 net 3/3] sctp: destroy bucket if failed to bind addr Mao Wenan @ 2019-09-13 20:06 ` David Miller 3 siblings, 0 replies; 19+ messages in thread From: David Miller @ 2019-09-13 20:06 UTC (permalink / raw) To: maowenan Cc: vyasevich, nhorman, marcelo.leitner, linux-sctp, netdev, linux-kernel, kernel-janitors From: Mao Wenan <maowenan@huawei.com> Date: Thu, 12 Sep 2019 12:02:16 +0800 > First two patches are to do cleanup, remove redundant assignment, > and change return type of sctp_get_port_local. > Third patch is to fix memory leak for sctp_do_bind if failed > to bind address. > > --- > v2: add one patch to change return type of sctp_get_port_local. Series applied with Fixes: tag removed from patch #1. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net 2/2] sctp: destroy bucket if failed to bind addr 2019-09-10 7:13 [PATCH net 0/2] fix memory leak for sctp_do_bind Mao Wenan 2019-09-10 7:13 ` [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local Mao Wenan @ 2019-09-10 7:13 ` Mao Wenan 2019-09-10 7:16 ` [PATCH net 0/2] fix memory leak for sctp_do_bind Neil Horman 2 siblings, 0 replies; 19+ messages in thread From: Mao Wenan @ 2019-09-10 7:13 UTC (permalink / raw) To: vyasevich, nhorman, marcelo.leitner, davem Cc: linux-sctp, netdev, linux-kernel, kernel-janitors, Mao Wenan, Hulk Robot There is one memory leak bug report: BUG: memory leak unreferenced object 0xffff8881dc4c5ec0 (size 40): comm "syz-executor.0", pid 5673, jiffies 4298198457 (age 27.578s) hex dump (first 32 bytes): 02 00 00 00 81 88 ff ff 00 00 00 00 00 00 00 00 ................ f8 63 3d c1 81 88 ff ff 00 00 00 00 00 00 00 00 .c=............. backtrace: [<0000000072006339>] sctp_get_port_local+0x2a1/0xa00 [sctp] [<00000000c7b379ec>] sctp_do_bind+0x176/0x2c0 [sctp] [<000000005be274a2>] sctp_bind+0x5a/0x80 [sctp] [<00000000b66b4044>] inet6_bind+0x59/0xd0 [ipv6] [<00000000c68c7f42>] __sys_bind+0x120/0x1f0 net/socket.c:1647 [<000000004513635b>] __do_sys_bind net/socket.c:1658 [inline] [<000000004513635b>] __se_sys_bind net/socket.c:1656 [inline] [<000000004513635b>] __x64_sys_bind+0x3e/0x50 net/socket.c:1656 [<0000000061f2501e>] do_syscall_64+0x72/0x2e0 arch/x86/entry/common.c:296 [<0000000003d1e05e>] entry_SYSCALL_64_after_hwframe+0x49/0xbe This is because in sctp_do_bind, if sctp_get_port_local is to create hash bucket successfully, and sctp_add_bind_addr failed to bind address, e.g return -ENOMEM, so memory leak found, it needs to destroy allocated bucket. Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Mao Wenan <maowenan@huawei.com> --- net/sctp/socket.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 766b68b55ebe..ab37fc1f7bb6 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -412,11 +412,13 @@ static int sctp_do_bind(struct sock *sk, union sctp_addr *addr, int len) ret = sctp_add_bind_addr(bp, addr, af->sockaddr_len, SCTP_ADDR_SRC, GFP_ATOMIC); - /* Copy back into socket for getsockname() use. */ - if (!ret) { - inet_sk(sk)->inet_sport = htons(inet_sk(sk)->inet_num); - sp->pf->to_sk_saddr(addr, sk); + if (ret) { + sctp_put_port(sk); + return ret; } + /* Copy back into socket for getsockname() use. */ + inet_sk(sk)->inet_sport = htons(inet_sk(sk)->inet_num); + sp->pf->to_sk_saddr(addr, sk); return ret; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH net 0/2] fix memory leak for sctp_do_bind 2019-09-10 7:13 [PATCH net 0/2] fix memory leak for sctp_do_bind Mao Wenan 2019-09-10 7:13 ` [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local Mao Wenan 2019-09-10 7:13 ` [PATCH net 2/2] sctp: destroy bucket if failed to bind addr Mao Wenan @ 2019-09-10 7:16 ` Neil Horman 2 siblings, 0 replies; 19+ messages in thread From: Neil Horman @ 2019-09-10 7:16 UTC (permalink / raw) To: Mao Wenan Cc: vyasevich, marcelo.leitner, davem, linux-sctp, netdev, linux-kernel, kernel-janitors On Tue, Sep 10, 2019 at 03:13:41PM +0800, Mao Wenan wrote: > First patch is to do cleanup, remove redundant assignment, > second patch is to fix memory leak for sctp_do_bind if failed > to bind address. > > Mao Wenan (2): > sctp: remove redundant assignment when call sctp_get_port_local > sctp: destroy bucket if failed to bind addr > > net/sctp/socket.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > -- > 2.20.1 > > Series Acked-by: Neil Horman <nhorman@tuxdriver.com> ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-09-13 20:07 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-10 7:13 [PATCH net 0/2] fix memory leak for sctp_do_bind Mao Wenan 2019-09-10 7:13 ` [PATCH net 1/2] sctp: remove redundant assignment when call sctp_get_port_local Mao Wenan 2019-09-10 18:57 ` Dan Carpenter 2019-09-10 19:22 ` Dan Carpenter 2019-09-11 1:30 ` maowenan 2019-09-11 8:30 ` Dan Carpenter 2019-09-11 14:30 ` Marcelo Ricardo Leitner 2019-09-11 14:39 ` Marcelo Ricardo Leitner 2019-09-12 2:05 ` maowenan 2019-09-12 4:02 ` [PATCH v2 net 0/3] fix memory leak for sctp_do_bind Mao Wenan 2019-09-12 4:02 ` [PATCH v2 net 1/3] sctp: change return type of sctp_get_port_local Mao Wenan 2019-09-12 14:51 ` Marcelo Ricardo Leitner 2019-09-12 4:02 ` [PATCH v2 net 2/3] sctp: remove redundant assignment when call sctp_get_port_local Mao Wenan 2019-09-12 14:52 ` Marcelo Ricardo Leitner 2019-09-12 4:02 ` [PATCH v2 net 3/3] sctp: destroy bucket if failed to bind addr Mao Wenan 2019-09-12 14:52 ` Marcelo Ricardo Leitner 2019-09-13 20:06 ` [PATCH v2 net 0/3] fix memory leak for sctp_do_bind David Miller 2019-09-10 7:13 ` [PATCH net 2/2] sctp: destroy bucket if failed to bind addr Mao Wenan 2019-09-10 7:16 ` [PATCH net 0/2] fix memory leak for sctp_do_bind Neil Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).