* [PATCH 1/2] net/x25: Fix x25_neigh refcnt leak when x25_connect() fails
@ 2020-04-23 5:13 Xiyu Yang
2020-04-23 6:36 ` Yuehaibing
2020-04-23 7:00 ` Martin Schiller
0 siblings, 2 replies; 4+ messages in thread
From: Xiyu Yang @ 2020-04-23 5:13 UTC (permalink / raw)
To: Andrew Hendry, David S. Miller, Jakub Kicinski, Martin Schiller,
Xiyu Yang, Eric Dumazet, Greg Kroah-Hartman, Thomas Gleixner,
linux-x25, netdev, linux-kernel
Cc: yuanxzhang, kjlu, Xin Tan
x25_connect() invokes x25_get_neigh(), which returns a reference of the
specified x25_neigh object to "x25->neighbour" with increased refcnt.
When x25_connect() returns, local variable "x25" and "x25->neighbour"
become invalid, so the refcount should be decreased to keep refcount
balanced.
The reference counting issue happens in one exception handling path of
x25_connect(). When sock state is not TCP_ESTABLISHED and its flags
include O_NONBLOCK, the function forgets to decrease the refcnt
increased by x25_get_neigh(), causing a refcnt leak.
Fix this issue by jumping to "out_put_neigh" label when x25_connect()
fails.
Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
---
net/x25/af_x25.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index d5b09bbff375..e6571c56209b 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -816,7 +816,7 @@ static int x25_connect(struct socket *sock, struct sockaddr *uaddr,
/* Now the loop */
rc = -EINPROGRESS;
if (sk->sk_state != TCP_ESTABLISHED && (flags & O_NONBLOCK))
- goto out;
+ goto out_put_neigh;
rc = x25_wait_for_connection_establishment(sk);
if (rc)
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] net/x25: Fix x25_neigh refcnt leak when x25_connect() fails
2020-04-23 5:13 [PATCH 1/2] net/x25: Fix x25_neigh refcnt leak when x25_connect() fails Xiyu Yang
@ 2020-04-23 6:36 ` Yuehaibing
2020-04-23 7:00 ` Martin Schiller
1 sibling, 0 replies; 4+ messages in thread
From: Yuehaibing @ 2020-04-23 6:36 UTC (permalink / raw)
To: Xiyu Yang, Andrew Hendry, David S. Miller, Jakub Kicinski,
Martin Schiller, Eric Dumazet, Greg Kroah-Hartman,
Thomas Gleixner, linux-x25, netdev, linux-kernel
Cc: yuanxzhang, kjlu, Xin Tan
On 2020/4/23 13:13, Xiyu Yang wrote:
> x25_connect() invokes x25_get_neigh(), which returns a reference of the
> specified x25_neigh object to "x25->neighbour" with increased refcnt.
>
> When x25_connect() returns, local variable "x25" and "x25->neighbour"
> become invalid, so the refcount should be decreased to keep refcount
> balanced.
>
> The reference counting issue happens in one exception handling path of
> x25_connect(). When sock state is not TCP_ESTABLISHED and its flags
> include O_NONBLOCK, the function forgets to decrease the refcnt
> increased by x25_get_neigh(), causing a refcnt leak.
>
> Fix this issue by jumping to "out_put_neigh" label when x25_connect()
> fails.
>
> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
> ---
> net/x25/af_x25.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
> index d5b09bbff375..e6571c56209b 100644
> --- a/net/x25/af_x25.c
> +++ b/net/x25/af_x25.c
> @@ -816,7 +816,7 @@ static int x25_connect(struct socket *sock, struct sockaddr *uaddr,
> /* Now the loop */
> rc = -EINPROGRESS;
> if (sk->sk_state != TCP_ESTABLISHED && (flags & O_NONBLOCK))
> - goto out;
> + goto out_put_neigh;
This seems not right, see
commit e21dba7a4df4 ("net/x25: fix nonblocking connect")
>
> rc = x25_wait_for_connection_establishment(sk);
> if (rc)
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] net/x25: Fix x25_neigh refcnt leak when x25_connect() fails
@ 2020-04-23 6:36 ` Yuehaibing
0 siblings, 0 replies; 4+ messages in thread
From: Yuehaibing @ 2020-04-23 6:36 UTC (permalink / raw)
To: Xiyu Yang, Andrew Hendry, David S. Miller, Jakub Kicinski,
Martin Schiller, Eric Dumazet, Greg Kroah-Hartman,
Thomas Gleixner, linux-x25, netdev, linux-kernel
Cc: yuanxzhang, kjlu, Xin Tan
On 2020/4/23 13:13, Xiyu Yang wrote:
> x25_connect() invokes x25_get_neigh(), which returns a reference of the
> specified x25_neigh object to "x25->neighbour" with increased refcnt.
>
> When x25_connect() returns, local variable "x25" and "x25->neighbour"
> become invalid, so the refcount should be decreased to keep refcount
> balanced.
>
> The reference counting issue happens in one exception handling path of
> x25_connect(). When sock state is not TCP_ESTABLISHED and its flags
> include O_NONBLOCK, the function forgets to decrease the refcnt
> increased by x25_get_neigh(), causing a refcnt leak.
>
> Fix this issue by jumping to "out_put_neigh" label when x25_connect()
> fails.
>
> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
> ---
> net/x25/af_x25.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
> index d5b09bbff375..e6571c56209b 100644
> --- a/net/x25/af_x25.c
> +++ b/net/x25/af_x25.c
> @@ -816,7 +816,7 @@ static int x25_connect(struct socket *sock, struct sockaddr *uaddr,
> /* Now the loop */
> rc = -EINPROGRESS;
> if (sk->sk_state != TCP_ESTABLISHED && (flags & O_NONBLOCK))
> - goto out;
> + goto out_put_neigh;
This seems not right, see
commit e21dba7a4df4 ("net/x25: fix nonblocking connect")
>
> rc = x25_wait_for_connection_establishment(sk);
> if (rc)
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] net/x25: Fix x25_neigh refcnt leak when x25_connect() fails
2020-04-23 5:13 [PATCH 1/2] net/x25: Fix x25_neigh refcnt leak when x25_connect() fails Xiyu Yang
2020-04-23 6:36 ` Yuehaibing
@ 2020-04-23 7:00 ` Martin Schiller
1 sibling, 0 replies; 4+ messages in thread
From: Martin Schiller @ 2020-04-23 7:00 UTC (permalink / raw)
To: Xiyu Yang
Cc: Andrew Hendry, David S. Miller, Jakub Kicinski, Eric Dumazet,
Greg Kroah-Hartman, Thomas Gleixner, linux-x25, netdev,
linux-kernel, yuanxzhang, kjlu, Xin Tan
On 2020-04-23 07:13, Xiyu Yang wrote:
> x25_connect() invokes x25_get_neigh(), which returns a reference of the
> specified x25_neigh object to "x25->neighbour" with increased refcnt.
>
> When x25_connect() returns, local variable "x25" and "x25->neighbour"
> become invalid, so the refcount should be decreased to keep refcount
> balanced.
>
> The reference counting issue happens in one exception handling path of
> x25_connect(). When sock state is not TCP_ESTABLISHED and its flags
> include O_NONBLOCK, the function forgets to decrease the refcnt
> increased by x25_get_neigh(), causing a refcnt leak.
>
> Fix this issue by jumping to "out_put_neigh" label when x25_connect()
> fails.
I don't agree with that.
Please have a look at commit e21dba7a4df4 ("net/x25: fix nonblocking
connect).
But I also think you are right and there seems to be a refcnt leak,
which should be fixed by a call to x25_neigh_put() in the
x25_disconnect() function.
- Martin
>
> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
> ---
> net/x25/af_x25.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
> index d5b09bbff375..e6571c56209b 100644
> --- a/net/x25/af_x25.c
> +++ b/net/x25/af_x25.c
> @@ -816,7 +816,7 @@ static int x25_connect(struct socket *sock, struct
> sockaddr *uaddr,
> /* Now the loop */
> rc = -EINPROGRESS;
> if (sk->sk_state != TCP_ESTABLISHED && (flags & O_NONBLOCK))
> - goto out;
> + goto out_put_neigh;
>
> rc = x25_wait_for_connection_establishment(sk);
> if (rc)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-23 7:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 5:13 [PATCH 1/2] net/x25: Fix x25_neigh refcnt leak when x25_connect() fails Xiyu Yang
2020-04-23 6:36 ` Yuehaibing
2020-04-23 6:36 ` Yuehaibing
2020-04-23 7:00 ` Martin Schiller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.