netdev - Re: [PATCH net] netfilter: unlock xt_table earlier in __do

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]

Message-ID: <CADvbK_eTNMqT_-fwcvUi8JRT-XTtz_bWMtZssCH_cGd7GxhR1A@mail.gmail.com>
Date:   Fri, 16 Feb 2018 12:25:56 +0100
From:   Xin Long <lucien.xin@...il.com>
To:     Florian Westphal <fw@...len.de>
Cc:     network dev <netdev@...r.kernel.org>,
        netfilter-devel@...r.kernel.org, davem <davem@...emloft.net>,
        Pablo Neira Ayuso <pablo@...filter.org>
Subject: Re: [PATCH net] netfilter: unlock xt_table earlier in __do_replace

On Fri, Feb 16, 2018 at 12:02 PM, Florian Westphal <fw@...len.de> wrote:
> Xin Long <lucien.xin@...il.com> wrote:
>> Now it's doing cleanup_entry for oldinfo under the xt_table lock,
>> but it's not really necessary. After the replacement job is done
>> in xt_replace_table, oldinfo is not used elsewhere any more, and
>> it can be freed without xt_table lock safely.
>
> Right.
>
>> The important thing is that rtnl_lock is called in some xt_target
>> destroy, which means rtnl_lock, a big lock is used in xt_table
>> lock, a smaller one. It usually could be the reason why a dead
>> lock may happen.
>
> In which cases do we aquire the xt table mutex from places that hold
> rtnl mutex?
Not really now.
But there was one, which though had been fixed in another way in:
https://patchwork.ozlabs.org/patch/870797/

I meant this kind of case (big lock used under small lock) have the
risk that may cause a dead lock.
Sorry for confusing.

>
>> Besides, all xt_target/match checkentry is called out of xt_table
>> lock. It's better also to move all cleanup_entry calling out of
>> xt_table lock, just as do_replace_finish does for ebtables.
>
> Agree but I don't see how this patch fixes a bug so I would prefer if
> this could simmer in nf-next first.
Sure. No bug fix, it's an improvement.