netdev - Re: [PATCH nf-next v2 2/3] netfilter: call nf_hook_state_init with rcu_read

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]

Date:	Thu, 14 Jul 2016 19:19:20 +0200
From:	Pablo Neira Ayuso <pablo@...filter.org>
To:	Aaron Conole <aconole@...heb.org>
Cc:	netdev@...r.kernel.org, netfilter-devel@...r.kernel.org,
	Florian Westphal <fw@...len.de>
Subject: Re: [PATCH nf-next v2 2/3] netfilter: call nf_hook_state_init with
 rcu_read_lock held

On Tue, Jul 12, 2016 at 11:32:20AM -0400, Aaron Conole wrote:
> From: Florian Westphal <fw@...len.de>
> 
> This makes things simpler because we can store the head of the list
> in the nf_state structure without worrying about concurrent add/delete
> of hook elements from the list.

This is something that you need for your follow up patch, right? Then
it would be good to document this here.

More comments below.

> diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
> index 9230f9a..ad444f0 100644
> --- a/include/linux/netfilter.h
> +++ b/include/linux/netfilter.h
> @@ -174,10 +174,16 @@ static inline int nf_hook_thresh(u_int8_t pf, unsigned int hook,
>  
>  	if (!list_empty(hook_list)) {
>  		struct nf_hook_state state;
> +		int ret;
>  
> +		/* We may already have this, but read-locks nest anyway */
> +		rcu_read_lock();
>  		nf_hook_state_init(&state, hook_list, hook, thresh,
>  				   pf, indev, outdev, sk, net, okfn);
> -		return nf_hook_slow(skb, &state);
> +
> +		ret = nf_hook_slow(skb, &state);
> +		rcu_read_unlock();
> +		return ret;
>  	}
>  	return 1;
>  }
> diff --git a/include/linux/netfilter_ingress.h b/include/linux/netfilter_ingress.h
> index 5fcd375..6965ba0 100644
> --- a/include/linux/netfilter_ingress.h
> +++ b/include/linux/netfilter_ingress.h
> @@ -14,6 +14,7 @@ static inline bool nf_hook_ingress_active(const struct sk_buff *skb)
>  	return !list_empty(&skb->dev->nf_hooks_ingress);
>  }
>  
> +/* caller must hold rcu_read_lock */
>  static inline int nf_hook_ingress(struct sk_buff *skb)
>  {
>  	struct nf_hook_state state;
> diff --git a/net/bridge/netfilter/ebt_redirect.c b/net/bridge/netfilter/ebt_redirect.c
> index 20396499..2e7c4f9 100644
> --- a/net/bridge/netfilter/ebt_redirect.c
> +++ b/net/bridge/netfilter/ebt_redirect.c
> @@ -24,7 +24,7 @@ ebt_redirect_tg(struct sk_buff *skb, const struct xt_action_param *par)
>  		return EBT_DROP;
>  
>  	if (par->hooknum != NF_BR_BROUTING)
> -		/* rcu_read_lock()ed by nf_hook_slow */
> +		/* rcu_read_lock()ed by nf_hook_thresh */

Why are all these comments being renamed in this patch? This patch
description doesn't say anything about this.