Florian Westphal <fw@xxxxxxxxx> wrote on 05/22/2012 08:08:58 PM:
Thanks for your review.
> >
> > NFQA_CFG_QUEUE_MAXLEN, /* __u32 */
> > + NFQA_CFG_MASK, /* identify which flags to change */
> > + NFQA_CFG_FLAGS, /* value of these flags (__u32) */
>
> __be32?
Yes, I will change this.
> I see that QUEUE_MAXLEN gets ntohl treatment, too....
>
> > +static void handle_fail_open(struct sk_buff *skb,
> > + int (*okfn)(struct sk_buff *))
> > +{
> > + struct sk_buff *segs;
> > + bool gso;
> > +
> > + segs = skb->next ? : skb;
> > + gso = skb->next != NULL;
> > +
> > + do {
> > + struct sk_buff *nskb = segs->next;
> > +
> > + segs->next = NULL;
> > + okfn(segs);
> > + segs = nskb;
> > + } while (segs);
> > +
> > + if (gso)
> > + kfree_skb(skb);
> > +}
>
> I don't understand why this is needed at all.
> Conceptually, what you're doing is identical to the
> --nfqueue-bypass feature, so it should be enough to change
Yes, I could use the same code. However I am not clear about the
ESRCH path since it doesn't seem to handle GSO skb correctly? See
nf_queue below where it calls kfree_skb even for -ESRCH:
if (err == 0) {
nf_bridge_adjust_segmented_data(segs);
err = __nf_queue(segs, elem, pf, hook, indev,
outdev, okfn, queuenum);
}
if (err == 0)
queued++;
else
kfree_skb(segs);
Maybe this needs a check for ESRCH and return back to hook_slow
to work correctly? If this is the case, I can submit a patch to
fix this, and piggy-back ESRCH for fail-open too.
> > @@ -199,10 +226,18 @@ next_hook:
> > if (err == -ESRCH &&
> > (verdict & NF_VERDICT_FLAG_QUEUE_BYPASS))
> > goto next_hook;
> > + if (err == -ENOSPC) {
> > + failopen = 1;
> > + goto next_hook;
> > + }
> > kfree_skb(skb);
>
> to
>
> if (err == -ENOSPC || (err == -ESRCH && (verdict &
> NF_VERDICT_FLAG_QUEUE_BYPASS))
> goto next_hook;
This would work if the skb was not GSO. So maybe the above
fix for ESRCH will let me share the code for fail-open too?
> [ or, take advantage of the existing -ECANCELED and have the queueing
> backend return that if queue is full and fail-open is enabled ]
>
> Yes, that means that if the userspace ruleset is
> -j NFQUEUE
> -j DROP
>
> then your packets will be dropped even if the userspace application
> enables failopen.
>
> But thats a feature, since you could also do
> -j NFQUEUE
> -m limt ... -j LOG --log-prefix "queue overflow"
>
> or play extra games wrt. "drop if established, CONNMARK
> for future bypass if --ctstate NEW", etc.
>
> If its a requirment for you that userspace can force ACCEPT
> regardless of ruleset, then perhaps it might be better to
> add a separate "default verdict" option and invoke
> nf_reinject() directly from the qeueueing backend instead
> of passing the fail-open information back to nf_hook_slow?
>
> > + flags = nla_data(nfqa[NFQA_CFG_FLAGS]);
> > + mask = nla_data(nfqa[NFQA_CFG_MASK]);
>
> nla_get_be32()?
> [ _u32 would make more sense to me, but other attributes are be32 too,
> so I'm ok with it ]
Yes, I will change to be32.
> > - if (err == 0)
> > +
> > + if (err == 0) {
> > queued++;
> > - else
> > + } else if (err == -ENOSPC) {
> > + /* Queue failed due to queue-full and handler is
> > + * in "fail-open" mode.
> > + */
> > + segs->next = nskb;
> > + skb->next = segs;
> > + break;
> > + } else {
> > kfree_skb(segs);
> > + }
> > segs = nskb;
> > } while (segs);
> >
> > - if (queued) {
> > + if (queued && err != -ENOSPC) {
> > kfree_skb(skb);
> > return 0;
> > }
>
> Similarily, this shouldn't be needed either any more since you
> no longer need to check for -ENOSPC (existing --queue-bypass behaviour
> should handle your case, too).
Slight change could be needed here, since queued is 0 for --queue-bypass
case (and hence return -ESRCH), while it could be >0 for fail-open,
where we still want to return error as some segments were not queued.
Thanks,
- KK
--
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
