[PATCH] appletalk: Fix potential refcount leak

[PATCH] appletalk: Fix potential refcount leak

[Liang He]

In atrtr_create(), we have added a dev_hold for the new reference.
However, based on the code, if the 'rt' is not NULL and its 'dev'
is not NULL, we should use dev_put() for the replaced reference.

Signed-off-by: Liang He <windhl@126.com>
---
 net/appletalk/ddp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index a06f4d4a6f47..7e317d6448d1 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -564,6 +564,7 @@ static int atrtr_create(struct rtentry *r, struct net_device *devhint)
 	/* Fill in the routing entry */
 	rt->target  = ta->sat_addr;
 	dev_hold(devhint);
+	dev_put(rt->dev);
 	rt->dev     = devhint;
 	rt->flags   = r->rt_flags;
 	rt->gateway = ga->sat_addr;
-- 
2.25.1

Re: [PATCH] appletalk: Fix potential refcount leak

[Eric Dumazet]

On Mon, Oct 24, 2022 at 8:25 AM Liang He <windhl@126.com> wrote:
>
> In atrtr_create(), we have added a dev_hold for the new reference.
> However, based on the code, if the 'rt' is not NULL and its 'dev'
> is not NULL, we should use dev_put() for the replaced reference.
>
> Signed-off-by: Liang He <windhl@126.com>
> ---
>  net/appletalk/ddp.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
> index a06f4d4a6f47..7e317d6448d1 100644
> --- a/net/appletalk/ddp.c
> +++ b/net/appletalk/ddp.c
> @@ -564,6 +564,7 @@ static int atrtr_create(struct rtentry *r, struct net_device *devhint)
>         /* Fill in the routing entry */
>         rt->target  = ta->sat_addr;
>         dev_hold(devhint);
> +       dev_put(rt->dev);
>         rt->dev     = devhint;
>         rt->flags   = r->rt_flags;
>         rt->gateway = ga->sat_addr;
>

IMO appletalk is probably completely broken.

atalk_routes_lock is not held while other threads might use rt->dev
and would not expect rt->dev to be changed under them
(atalk_route_packet() )

I would vote to remove it completely, unless someone is willing to
test any change in it.

Re: [PATCH] appletalk: Fix potential refcount leak

[Jakub Kicinski]

On Mon, 24 Oct 2022 08:36:13 -0700 Eric Dumazet wrote:
> IMO appletalk is probably completely broken.
> 
> atalk_routes_lock is not held while other threads might use rt->dev
> and would not expect rt->dev to be changed under them
> (atalk_route_packet() )
> 
> I would vote to remove it completely, unless someone is willing to
> test any change in it.

+1 for killing all of appletalk.

Arnd, I think you suggested the removal in the past as well, or were
you just saying to remove localtalk ?

Re: [PATCH] appletalk: Fix potential refcount leak

[Arnd Bergmann]

On Mon, Oct 24, 2022, at 23:50, Jakub Kicinski wrote:
> On Mon, 24 Oct 2022 08:36:13 -0700 Eric Dumazet wrote:
>> IMO appletalk is probably completely broken.
>> 
>> atalk_routes_lock is not held while other threads might use rt->dev
>> and would not expect rt->dev to be changed under them
>> (atalk_route_packet() )
>> 
>> I would vote to remove it completely, unless someone is willing to
>> test any change in it.
>
> +1 for killing all of appletalk.
>
> Arnd, I think you suggested the removal in the past as well, or were
> you just saying to remove localtalk ?

As far as I can tell, there were no objections to removing localtalk,
and definite upsides to removing the last such driver (CONFIG_COPS).
Similarly, it seems that IPDDP (IP tunneled through appletalk) can
probably go, even though it does not depend on specific hardware.

According to Doug Brown, only ethertalk is used in practice, but
there are definitely users of that. See [1] for the thread from
when this came up last.

     Arnd

[1] https://lore.kernel.org/netdev/9cac4fbd-9557-b0b8-54fa-93f0290a6fb8@schmorgal.com/