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

[Konstantin Khorenko]

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.

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