Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> I've got some questions and comments on this patchset, please see below.
Thank you for spending time on reviewing this patchset!
> On 27/12/10 00:58, Florian Westphal wrote:
> > instead of returning -1 on error, return an error number to allow the
> > caller to handle some errors differently.
> >
> > To make things simpler try_module_get() failure is no longer special-cased.
> >
> > The new return values will be used in followup patch.
> >
> > Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
> > --- a/net/netfilter/core.c
> > +++ b/net/netfilter/core.c
> > @@ -179,9 +179,8 @@ next_hook:
> > if (ret == 0)
> > ret = -EPERM;
> > } else if ((verdict & NF_VERDICT_MASK) == NF_QUEUE) {
> > - if (!nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
> > - verdict >> NF_VERDICT_BITS))
> > - goto next_hook;
> > + nf_queue(skb, elem, pf, hook, indev, outdev, okfn,
> > + verdict >> NF_VERDICT_BITS);
>
> You have to remove the next_hook label if you want to do this.
>
> It's not clear to me why we need to remove the goto jump, could you
> clarify this?
Sure. The only place where nf_queue returns 0 is this snippet in __nf_queue():
> > - /* If it's going away, ignore hook. */
> > - if (!try_module_get(entry->elem->owner)) {
> > - rcu_read_unlock();
> > - kfree(entry);
> > - return 0;
> > - }
> > + if (!try_module_get(entry->elem->owner))
> > + goto err_unlock;
>
> With this change, we're now releasing the skb, is this deliberate?
I think its not necessary to make this a special case (i.e. just
kfree_skb(skb) instead).
An alternative is to decide on a meaningful errno value that
could be used as return value to signal this condition.
> > #ifdef CONFIG_BRIDGE_NETFILTER
> > struct net_device *physindev;
> > @@ -136,8 +136,10 @@ static int __nf_queue(struct sk_buff *skb,
> > goto err_unlock;
>
> here above there's the following:
>
> qh = rcu_dereference(queue_handler[pf]);
> if (!qh)
> goto err_unlock;
>
> afinfo = nf_get_afinfo(pf);
> if (!afinfo)
> goto err_unlock;
>
> With your patch, we're returning -EINVAL. I think it's better to return
> -ENOENT.
Agreed.
> > @@ -218,18 +218,26 @@ int nf_queue(struct sk_buff *skb,
> > segs = skb_gso_segment(skb, 0);
> > kfree_skb(skb);
> > if (IS_ERR(segs))
> > - return 1;
> > + return -ENOMEM;
>
> Better propagate the error of skb_gso_segment(...) with PTR_ERR()
The reason I did not do that is because I wanted to limit the number
of code one has to check for possible errno values that can be returned
by nf_queue.
Its probably reasonable to assume that skb_gso_segment() won't return
ESRCH (which I use to determine that qeueuing failed because the queue number did
not exist), so it might be safe to use PTR_ERR here.
What do you think?
> > + queued = 0;
> > do {
> > struct sk_buff *nskb = segs->next;
> >
> > segs->next = NULL;
> > - if (!__nf_queue(segs, elem, pf, hook, indev, outdev, okfn,
> > - queuenum))
> > - kfree_skb(segs);
> > + if (error == 0) {
> > + error = __nf_queue(segs, elem, pf, hook, indev,
> > + outdev, okfn, queuenum);
> > + if (error == 0)
> > + queued++;
> > + }
> > + kfree_skb(segs);
>
> Before this patch, segs were only released if try_module_get() failed.
> Now it always release it?
Yes, my bad.
I'll add it to the list of things to fix up. Thanks for catching this.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
