[Devel] [PATCH RH7] netfilter: drop/reaquire nfnl_lock on request_module() in nft_log module

[Konstantin Khorenko]

On 07/16/2020 07:47 PM, Konstantin Khorenko wrote:
> On 07/16/2020 04:55 PM, Andrey Ryabinin wrote:
>> On 7/16/20 3:37 PM, Konstantin Khorenko wrote:
>>>
>>> +extern struct nf_logger __rcu *loggers[NFPROTO_NUMPROTO][NF_LOG_TYPE_MAX] __read_mostly;
>>> +
>>> +/*
>>> + * In "nft_log" module we need to drop nfnl lock while performing
>>> + * request_module(), calls to nf_logger_find_get() in other
>>> + * modules are done without nfnl_lock taken.
>>> + *
>>> + * nf_logger_find_get
>>> + *  nft_log_init
>>> + *   nf_tables_newexpr
>>> + *    nf_tables_newrule		// nc->call_batch
>>> + *				// called with nfnl_lock taken
>>> + *
>>> + *     nfnetlink_rcv_batch	// takes nfnl_lock(NFNL_SUBSYS_NFTABLES)
>>> + *      nfnetlink_rcv
>>> + *       netlink_unicast
>>> + *        netlink_sendmsg
>>> + */
>>> +static int nf_logger_find_get_lock(int pf, enum nf_log_type type)
>>> +{
>>> +	struct nf_logger *logger;
>>> +	int ret = 0;
>>> +
>>> +	logger = loggers[pf][type];
>>> +	if (logger == NULL) {
>>> +		nfnl_unlock(NFNL_SUBSYS_NFTABLES);
>>
>> nfnetlink_rcv_batch()  takes nfnl_lock(subsys_id) lock
>> How you can be sure that subsys_id is always NFNL_SUBSYS_NFTABLES ?
>
> 1) nf_logger_find_get_lock()	is called in only 1 place - in nft_log_init()
> 2) nft_log_init() == nft_log_ops.init() == struct nft_expr_ops::init()
>     struct nft_expr_ops::init()	is called only in nf_tables_newexpr()
>     => nft_log_init()		is called only in nf_tables_newexpr()
>
> 3) nf_tables_newexpr()		is called only in nf_tables_newrule() and in nft_expr_init()
>
> a) nft_expr_init() is called only in nft_dynset_init()
>     nft_dynset_init() == nft_dynset_ops.init() == struct nft_expr_ops::init()
>
>     struct nft_expr_ops::init()	is called only in nf_tables_newexpr()
>     => nft_dynset_init()		is called only in nf_tables_newexpr()
>     => goto 3)
>
> b) nf_tables_newrule() is the nf_tables_cb[NFT_MSG_NEWRULE].call_batch()
>     struct nfnl_callback nf_tables_cb[NFT_MSG_MAX];
>
>     struct nfnl_callback::call_batch() is called only in nfnetlink_rcv_batch() (nc->call_batch())
>     =>
>     b.1) nf_tables_newrule()	is called only in nfnetlink_rcv_batch()
>     b.2) nfnetlink_rcv_batch()	acquires nfnl_lock(NFNL_SUBSYS_NFTABLES ???)
> ===========
>                  /* Work around old nft using host byte order */
>                  if (nfgenmsg->res_id == NFNL_SUBSYS_NFTABLES)
>                          res_id = NFNL_SUBSYS_NFTABLES;
>                  else
>                          res_id = ntohs(nfgenmsg->res_id);
>                  nfnetlink_rcv_batch(skb, nlh, res_id);
> ===========
>
> Well, looks like i was not patient enough,
> i've proven that nfnl_lock() is always taken by above
> (i mean there is no path nf_logger_find_get() is called without nfnl_lock() is taken),
> but you are right, it can be not nfnl_lock(NFNL_SUBSYS_NFTABLES) but for some other subsys_id.
>
> Thank you for catching this, i will rework the patch.

Nope, i've recalled my checks!

static const struct nfnetlink_subsystem nf_tables_subsys = {
         .name           = "nf_tables",
         .subsys_id      = NFNL_SUBSYS_NFTABLES,
         .cb             = nf_tables_cb,
...

static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {
         [NFT_MSG_NEWRULE] = {
                 .call_batch     = nf_tables_newrule,
         },
...

nfnetlink_find_client(u_int16_t type, const struct nfnetlink_subsystem *ss)
{
         u_int8_t cb_id = NFNL_MSG_TYPE(type);

         return &ss->cb[cb_id];
}

nfnetlink_rcv_batch(subsys_id)
{
...
         nfnl_lock(subsys_id);
         ss = rcu_dereference_protected(table[subsys_id].subsys, ...
...
                 /* We only accept a batch with messages for the same
                  * subsystem.
                  */
                 if (NFNL_SUBSYS_ID(type) != subsys_id) {
                         err = -EINVAL;
                         goto ack;
                 }

                 nc = nfnetlink_find_client(type, ss);
...
			nc->call_batch()



If nc->call_batch() == nf_tables_newrule(), it means
nc == nf_tables_cb[NFT_MSG_NEWRULE]

nc == &ss->cb[cb_id]
=> ss == nf_tables_subsys

and we know that nf_tables_subsys.subsys_id = NFNL_SUBSYS_NFTABLES


static const struct nfnetlink_subsystem nf_tables_subsys = {
         .name           = "nf_tables",
         .subsys_id      = NFNL_SUBSYS_NFTABLES,
         .cb             = nf_tables_cb,
...


Proven?

>>> +		request_module("nf-logger-%u-%u", pf, type);
>>> +		nfnl_lock(NFNL_SUBSYS_NFTABLES);
>>> +		ret = -EAGAIN;
>>> +	}
>> .
>>