Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> On 12/01/11 21:49, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> >>> - /* 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.
Okay. I used -ECANCELED here, its rarely used in the kernel and
i think it comes closest.
> >> 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.
I added a comment about this and plan to send v2 of the patch set on
sunday.
Thanks again for reviewing,
Florian
--
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
