netdev - Re: macvlan: Fix device ref leak when purging bc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 21 Apr 2017 12:23:54 -0700
From:   Mahesh Bandewar (महेश बंडेवार) 
        <maheshb@...gle.com>
To:     Joe.Ghalam@...l.com
Cc:     herbert@...dor.apana.org.au, David Miller <davem@...emloft.net>,
        Clifford.Wichmann@...l.com, linux-netdev <netdev@...r.kernel.org>
Subject: Re: macvlan: Fix device ref leak when purging bc_queue

On Fri, Apr 21, 2017 at 7:40 AM,  <Joe.Ghalam@...l.com> wrote:
> That's not true. macvlan_dellink() unregisters the queue, and macvlan_process_broadcast() will never get called. Please note that I'm not speculating. I have traced enabled on the dev_put and dev_hold, and I'm reporting a real, reproducible issue.
> Her is a sequence of calls logged, when the issue happens. macvlan_process_broadcast() never happens.
>
> Apr 19 04:35:39 OS10 kernel: e101-001-0.v257: dev_put 16 dst_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 dev_get_by_index
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 do_ip_setsockopt
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 dst_alloc
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 dst_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 macvlan_broadcast_enqueue
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 macvlan_process_broadcast
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 macvlan_broadcast_enqueue
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 macvlan_process_broadcast
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 macvlan_broadcast_enqueue    <---insert
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 neigh_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 15 neigh_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 14 neigh_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 13 dst_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 12 dst_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 11 __netdev_adjacent_dev_remove <--- macvlan_dellink()
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 10 __netdev_adjacent_dev_remove <--- macvlan_dellink()
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 9 neigh_parms_release
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 8 neigh_parms_release
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 7 in6_dev_finish_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 6 rx_queue_release
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 5 netdev_queue_release
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 4 rollback_registered_many
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 3 free_fib_info_rcu
>
May be the system is busy and snapshot is too small, and eventually
process_broadcast() should get called. Deleting a slave does nothing
about cancelling the work-queue so it would happen eventually.

The change that Herbert proposed is correct. When packets are enqueued
for processing later a dev reference is taken and it's removed when
it's processed when it gets scheduled. The backlog is per port so it
makes sense to remove reference(s) before purging the queue prior to
deleting the port.

> ________________________________________
> From: Herbert Xu <herbert@...dor.apana.org.au>
> Sent: Thursday, April 20, 2017 9:40 PM
> To: Ghalam, Joe
> Cc: davem@...emloft.net; Wichmann, Clifford; netdev@...r.kernel.org
> Subject: Re: macvlan: Fix device ref leak when purging bc_queue
>
> On Thu, Apr 20, 2017 at 04:09:56PM +0000, Joe.Ghalam@...l.com wrote:
>> I agree with this change, but the same purge would be needed for the macvlan_dellink() call also.
>
> I don't think that's necessary because as long as the master
> device is still around it will continue to process the broadcast
> queue, thus removing any reference counts held.
>
> It's only when the queue is purged that we run into trouble.
>
> Cheers,
> --
> Email: Herbert Xu <herbert@...dor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt