| Message ID | 20160513200442.GA29941@breakpoint.cc |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Florian Westphal <fw@strlen.de> writes: > Eric W. Biederman <ebiederm@xmission.com> wrote: >> > AFAICS no other callers do something similar, but yes, >> > we'd need this all over the place if there are others. >> > >> > Maybe we need a saner fix, e.g. by adding bounds check to net_generic(), >> > and making sure that net_generic() returns non-NULL only if the per >> > netns memory was allocated properly. >> >> As a first approximiation I am thinking we should fix by making >> nf_queue_register_handler a per netns operation. > > We can do that but then we'd end up storing the very same address > for each namespace which I think isn't nice either. Sort of. At the same time we fundamentally need a per namespace variable to ask if things were initialized. So using the address as that variable changes the equation not at all, and keeps the code simpler. >> Either that or give nfnetlink_queue it's own dedicated place in >> struct net for struct nfnl_queue_net. > > That would work too, but then I don't see why that is preferrable > to this proposed patch. We could add a helper for this so that > we have something like > > static bool netns_was_inited(const struct net *n) > { > return net->list.next != NULL; > } That does not work because the real question were the things this piece of code cares about initialized. > Another alternative is this patch (not even compile tested, just to > illustrate the idea): Very much no. I believe that changing net_generic to return a value so that nfqnl_nf_hook_drop can detect if the timing is wrong is an error. I also don't think your changes to net_generic will work as the number of pointers should have been allocted properly from the start. Your proposed change is just papering over the problem and fixing it at the wrong level. (The obvious level yes) but still the wrong level. > What do you think? Eric
Eric W. Biederman <ebiederm@xmission.com> wrote: > Florian Westphal <fw@strlen.de> writes: > > > Eric W. Biederman <ebiederm@xmission.com> wrote: > >> > AFAICS no other callers do something similar, but yes, > >> > we'd need this all over the place if there are others. > >> > > >> > Maybe we need a saner fix, e.g. by adding bounds check to net_generic(), > >> > and making sure that net_generic() returns non-NULL only if the per > >> > netns memory was allocated properly. > >> > >> As a first approximiation I am thinking we should fix by making > >> nf_queue_register_handler a per netns operation. > > > > We can do that but then we'd end up storing the very same address > > for each namespace which I think isn't nice either. > > Sort of. At the same time we fundamentally need a per namespace > variable to ask if things were initialized. So using the address as > that variable changes the equation not at all, and keeps the code > simpler. We already do this for nf_conntrack_event_cb and nf_expect_event_cb so we store 16 byte per netns where one bit would suffice. > >> Either that or give nfnetlink_queue it's own dedicated place in > >> struct net for struct nfnl_queue_net. > > > > That would work too, but then I don't see why that is preferrable > > to this proposed patch. We could add a helper for this so that > > we have something like Addendum: its also a non-starter as nfqueue data is quite big and the module won't be loaded in most configurations. Looking at 70e9942f17a6193e9172a804e6569a8806633d6b again it seems the problem is somewhat related to this one, although not identical. Perhaps we should consider adding nf_netns_feature_ok(struct net *, enum nfnet_feature); enum nfnet_feature { NFNET_QUEUE, NFNET_EVENT, }; and then set/unset that from the netns init/exit handlers. nf_netns_feature_ok would then just need one bit in struct nf_net to store wheter whatever feature we care about is available. We could then de-netnsify those pointers again. > > static bool netns_was_inited(const struct net *n) > > { > > return net->list.next != NULL; > > } > > That does not work because the real question were the things this piece > of code cares about initialized. Hmm, AFAIU we can't have packets queued without the netns being on the list, no? > Very much no. > > I believe that changing net_generic to return a value so that > nfqnl_nf_hook_drop can detect if the timing is wrong is an error. Well, I don't see how this is fixable without having some means of detecting wheter 'timing' is wrong... You could say that making calls that end up in nfnetlink_queue without a successful ->init() is illegal but in that case you will need some condition by which the netfilter core can know wheter that call is ok in the first place. So I do not think that detecting it within nfnetlink_queue is worse than detecting this in the caller, in fact, detecting it in callee is better since the caller can't forget the test... > I also don't think your changes to net_generic will work as the number > of pointers should have been allocted properly from the start. As I said I did not test it. I only saw that net_assign_generic() can expand this array so I figured one has to check if ptr->[id] is useable. > Your proposed change is just papering over the problem and fixing it at > the wrong level. (The obvious level yes) but still the wrong level. Well, I have no more ideas. Looking at the number of function pointers used within netfilter I'm worried that we will end up pushing more and more redundant info into struct net to solve this "problem".
diff --git a/include/net/netns/generic.h b/include/net/netns/generic.h --- a/include/net/netns/generic.h +++ b/include/net/netns/generic.h @@ -38,7 +38,11 @@ static inline void *net_generic(const struct net *net, int id) rcu_read_lock(); ng = rcu_dereference(net->gen); - ptr = ng->ptr[id - 1]; + + if (ng->len < id) + ptr = ng->ptr[id - 1]; + else + ptr = NULL; rcu_read_unlock(); return ptr; diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c --- a/net/core/net_namespace.c +++ b/net/core/net_namespace.c @@ -111,6 +111,7 @@ static int ops_init(const struct pernet_operations *ops, struct net *net) return 0; cleanup: + net_assign_generic(net, *ops->id, NULL); kfree(data); out: diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c --- a/net/netfilter/nfnetlink_queue.c +++ b/net/netfilter/nfnetlink_queue.c @@ -927,6 +927,9 @@ static void nfqnl_nf_hook_drop(struct net *net, struct nf_hook_ops *hook) struct nfnl_queue_net *q = nfnl_queue_pernet(net); int i; + if (!q) + return; + rcu_read_lock(); for (i = 0; i < INSTANCE_BUCKETS; i++) { struct nfqnl_instance *inst;