On Mon, May 14, 2018 at 07:26:54PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote: > > static int __init nf_nat_init(void) > > diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c > > index 74a04638ef03..28e4fae98f60 100644 > > --- a/net/netfilter/nfnetlink_queue.c > > +++ b/net/netfilter/nfnetlink_queue.c > > @@ -227,6 +227,30 @@ find_dequeue_entry(struct nfqnl_instance *queue, unsigned int id) > > return entry; > > } > > > > +static void nfqnl_reinject(struct nf_queue_entry *entry, unsigned int verdict) > > +{ > > + enum ip_conntrack_info ctinfo; > > + struct nf_ct_hook *ct_hook; > > + struct nf_conn *ct; > > + int err; > > + > > + ct = nf_ct_get(entry->skb, &ctinfo); > > + if (ct && !nf_ct_is_confirmed(ct) && > > + verdict != NF_STOLEN && verdict != NF_DROP) { > > Why not verdict == NF_ACCEPT? We also have to deal with NF_STOP, right? > > static void > > nfqnl_flush(struct nfqnl_instance *queue, nfqnl_cmpfn cmpfn, unsigned long data) > > { > > @@ -237,7 +261,7 @@ nfqnl_flush(struct nfqnl_instance *queue, nfqnl_cmpfn cmpfn, unsigned long data) > > if (!cmpfn || cmpfn(entry, data)) { > > list_del(&entry->list); > > queue->queue_total--; > > - nf_reinject(entry, NF_DROP); > > + nfqnl_reinject(entry, NF_DROP); > > } > > } > > spin_unlock_bh(&queue->lock); > > @@ -686,7 +710,7 @@ __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue, > > err_out_unlock: > > spin_unlock_bh(&queue->lock); > > if (failopen) > > - nf_reinject(entry, NF_ACCEPT); > > + nfqnl_reinject(entry, NF_ACCEPT); > > I think failopen can use nf_reinject since we didn't queue packet to > userspace. Yes, that's fine. I just used nfqnl_reinject() for consistency, so all code uses it. > > @@ -1208,7 +1233,7 @@ static int nfqnl_recv_verdict(struct net *net, struct sock *ctnl, > > if (nfqa[NFQA_MARK]) > > entry->skb->mark = ntohl(nla_get_be32(nfqa[NFQA_MARK])); > > > > - nf_reinject(entry, verdict); > > + nfqnl_reinject(entry, verdict); > > I wonder if we should make nfqnl_reinject dependent on > nfqa[NFQA_PAYLOAD] ? > > (i.e., should we munge payload in case userspcae already did so?) You mean, skip this codepath if nfqa[NFQA_PAYLOAD] is set, right? Given this may be a userspace helper doing packet mangling? -- 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