On Thu, Aug 09, 2018 at 06:36:31PM +0300, Evgenii Shatokhin wrote: > On 07.08.2018 19:24, Josh Poimboeuf wrote: > > On Tue, Aug 07, 2018 at 06:13:29PM +0300, Evgenii Shatokhin wrote: > > > Hi, > > > > > > If there are any ideas how to safely apply fixes like > > > http://patchwork.ozlabs.org/patch/952304/ ("netfilter: ipset: fix > > > ip_set_list allocation failure") via a live patch, it would be great. > > > > > > Essentially, the patch replaces the calls to kmalloc and kfree with > > > kvmalloc > > > and kvfree, respectively: > > > ---------------------- > > > @@ -2059,7 +2059,7 @@ ip_set_net_init(struct net *net) > > > if (inst->ip_set_max >= IPSET_INVALID_ID) > > > inst->ip_set_max = IPSET_INVALID_ID - 1; > > > > > > - list = kcalloc(inst->ip_set_max, sizeof(struct ip_set *), GFP_KERNEL); > > > + list = kvcalloc(inst->ip_set_max, sizeof(struct ip_set *), GFP_KERNEL); > > > if (!list) > > > return -ENOMEM; > > > inst->is_deleted = false; > > > @@ -2087,7 +2087,7 @@ ip_set_net_exit(struct net *net) > > > } > > > } > > > nfnl_unlock(NFNL_SUBSYS_IPSET); > > > - kfree(rcu_dereference_protected(inst->ip_set_list, 1)); > > > + kvfree(rcu_dereference_protected(inst->ip_set_list, 1)); > > > } > > > > > > static struct pernet_operations ip_set_net_ops = { > > > ---------------------- > > > > > > However, this patch is unsafe to apply as is in runtime. Consider the > > > following scenario: > > > > > > 1. The live patch is loaded. > > > 2. The user creates a new net ns or whatever, so that the *patched* > > > ip_set_net_init() is called and calls kvcalloc(). Let us assume kvcalloc() > > > uses vmalloc() this time. > > > 3. The live patch is unloaded for some reason. > > > 4. The user destroys the net ns and the *original* ip_set_net_exit() is > > > called. The address of the memory area allocated with vmalloc at step 2 > > > will > > > be passed to kfree() here. The kernel will crash. > > > > > > Currently, I cannot see how to provide such fixes in a live patch without > > > additional hacks. > > > > > > As for the hacks, I could probably add a pre-patch callback that finds the > > > call to kfree() in the original ip_set_net_exit() and permanently replaces > > > it with a call to kvfree(). This way, even after the live patch has been > > > unloaded at step 3, the memory will be freed correctly at the step 4. Not > > > very elegant, but might work. > > > > > > As I am only interested in doing this on x86_64 right now, finding the > > > needed call in the binary code of ip_set_net_exit() is not a problem. As > > > for > > > replacement itself - well, I guess something like text_poke* or a part of > > > its implementation could be used here. > > > > > > Still, I wonder if there is a better way to apply such fixes in runtime, > > > ideally, without additional hacks. > > > > > > Any ideas? > > > > I'm thinking the only non-hackish way to prevent the above scenario is > > to prevent the module from being unloaded. Maybe with a > > try_module_get(THIS_MODULE) from the pre-patch hook? > > Thanks for a quick reply! > > Yes, it is possible to make the patch module unloadable but I guess, it > might make it more difficult to update the patch in the future, in our > particular case. > > Splitting this fix into a separate binary patch and making it permanently > loaded might be feasible too. It is essentially similar to a permanent > replacement of the calls in the binary code I described before, but probably > easier to implement/maintain. > > > > > However, I think there are also other races: > > > > 1) Before the patch, a netns is created, and the unpatched > > ip_set_net_init() is called, which uses kcalloc(). > > > > After the patch, the netns is destroyed, and the patched > > ip_set_net_exit() is called, which uses kvfree(). Boom. > > I think, this case is OK: kvfree() checks how memory was allocated. If > kmalloc was used, via kvmalloc or directly, kvfree() will call kfree(), > which is what is needed.
Ah, yes, you're right. > > The solution for #2 is a little trickier. One option might be to grab > > the appropriate lock (net->nsid_lock?) in the pre-patch callback and > > release it in the post-patch callback. > > Neat. Haven't thought about that. > > > > > Another option might be to split it into two patches, with the exit > > patch loaded first and the init patch loaded second. > > The "exit" patch should remain permanently loaded, of course, for that to > work. Or, at least, until we can guarantee no more netns'es are going to be > destroyed. > > Such multi-stage patching looks interesting. Need to think about it, esp., > about how it would do w.r.t. patch updates and downgrades. Let me know how it goes. This kind of thing might be interesting to discuss at LPC this year, if the livepatch microconference gets approved. -- Josh _______________________________________________ kpatch mailing list [email protected] https://www.redhat.com/mailman/listinfo/kpatch
