On 12/01/11 21:49, Florian Westphal wrote:
> 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!
welcome Florian ;-)
>> 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():
I see, only the try_module_get part.
>>> - /* 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).
This case is rare but I'd prefer not to drop packets if the hook is
going away.
[...]
>> 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.
Currently, it only return -EINVAL. Anyway, it makes sense what you
mention. Better use -EINVAL and add a short comment upon it that tells
why we're doing 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
