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
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
>>> ---
>>> 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
>
>
> .
>
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 <[email protected]>
> >>> ---
> >>> 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
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 <[email protected]>
> > >>> ---
> > >>> 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
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 <[email protected]>
> > > >>> ---
> > > >>> 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
>
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 <[email protected]>
>>>>>>> ---
>>>>>>> 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
>>
>
> .
>
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
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 <[email protected]>
Acked-by: Neil Horman <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Mao Wenan <[email protected]>
Acked-by: Neil Horman <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Thanks.
> Signed-off-by: Mao Wenan <[email protected]>
> ---
> 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
>
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 <[email protected]>
> Acked-by: Neil Horman <[email protected]>
Acked-by: Marcelo Ricardo Leitner <[email protected]>
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 <[email protected]>
> Signed-off-by: Mao Wenan <[email protected]>
> Acked-by: Neil Horman <[email protected]>
Acked-by: Marcelo Ricardo Leitner <[email protected]>
From: Mao Wenan <[email protected]>
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.