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

[Konstantin Khorenko]

On 07/17/2020 03:39 PM, Andrey Ryabinin wrote:
>
>
> On 7/16/20 3:37 PM, Konstantin Khorenko wrote:
>> +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);
>> +		request_module("nf-logger-%u-%u", pf, type);
>> +		nfnl_lock(NFNL_SUBSYS_NFTABLES);
>> +		ret = -EAGAIN;
>
> nfnetlink_rcv_batch() has special error path for EAGAIN:
> 			/* The lock was released to autoload some module, we
> 			 * have to abort and start from scratch using the
> 			 * original skb.
> 			 */
> 			if (err == -EAGAIN) {
> 				status |= NFNL_BATCH_REPLAY;
> 				goto done;
> 			}
>
>
> Yet, nf_logger_find_get_lock() don't return immediately with EAGAIN, but continues and potentially replaces EAGAIN with ENOENT.
> Why?

Well, this is the original logic, i did not change it.

The idea is:
* if loggers[pf][type] is not registered, try to load the appropriate module
   // the module load could fail, we keep it in mind

* next check loggers[pf][type] once again - and if it's empty again after our attempt to load the module
   (and thus to register desired loggers[pf][type]),
   then we don't need to return -EAGAIN, because
   - next module load attempt could also fail to register loggers[pf][type] and we get in a loop
   - we just don't want to try loading module more than once

=> in case loggers[pf][type] is still empty after single module load attempt =>
    something is deadly wrong, return -ENOENT, no need to probe it once more

- another case - try_module_get() fails - we also don't want to retry in this case

>> +	}
>> +
>> +	rcu_read_lock();
>> +	logger = rcu_dereference(loggers[pf][type]);
>> +
>> +	if ((logger == NULL) ||
>> +	    ((ret == 0) && !try_module_get(logger->me))) {
>> +		ret = -ENOENT;
>> +	}
>> +	rcu_read_unlock();
>> +	return ret;
>> +}
>> +
> .
>