On Tue, Jul 12, 2016 at 11:32:20AM -0400, Aaron Conole wrote:
> From: Florian Westphal <[email protected]>
>
> 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.
