Re: [PATCH 1/4] netfilter: use a linked list of loggers.

[Patrick McHardy <kaber@trash.net>]

Eric Leblond wrote:
> +/* return EEXIST if the same logger is registred, 0 on success. */
> +int nf_log_register(u_int8_t pf, struct nf_logger *logger)
>  {
>  	int ret;
> +	const struct nf_logger *llog;
>  
>  	if (pf >= ARRAY_SIZE(nf_loggers))
>  		return -EINVAL;
>  
> -	/* Any setup of logging members must be done before
> -	 * substituting pointer. */
> -	ret = mutex_lock_interruptible(&nf_log_mutex);
> -	if (ret < 0)
> -		return ret;
> +	mutex_lock(&nf_log_mutex);
>  
> -	if (!nf_loggers[pf])
> -		rcu_assign_pointer(nf_loggers[pf], logger);
> -	else if (nf_loggers[pf] == logger)
> -		ret = -EEXIST;
> -	else
> -		ret = -EBUSY;
> +	ret = -EEXIST;
> +	if (pf == NFPROTO_UNSPEC) {
> +		int i;
> +		for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
> +			if (__find_logger(i, logger->name) == NULL) {
> +				ret = 0;
> +				list_add_tail(&(logger->list[i]),
> +						&(nf_loggers_l[i]));
> +			}
> +	} else {
> +		if (__find_logger(pf, logger->name) == NULL) {
> +			ret = 0;
> +			/* register at end of list to honor first register win */
> +			list_add_tail(&(logger->list[pf]),
> +					  &nf_loggers_l[pf]);
> +			llog = rcu_dereference(nf_loggers[pf]);
> +			if (llog == NULL)
> +				rcu_assign_pointer(nf_loggers[pf], logger);
> +		}
> +	}

As the loggers are only registered statically by kernel modules,
I think this check is overkill. Just add them and let it crash
if someone does two registrations of the same logger, it won't
take long to notice. Similar for unregistration, its not worth
the trouble to check whether its actually registered, especially
since the list deletion doesn't even need to know. Just NULL
out the affected nf_loggers pointers and it should be fine.

Sorry for not saying this earlier.