* [help] Confusion on livepatch's per-task consistency model @ 2020-03-02 8:45 JeffleXu 2020-03-02 10:12 ` Petr Mladek 2020-03-02 10:36 ` Nicolai Stange 0 siblings, 2 replies; 6+ messages in thread From: JeffleXu @ 2020-03-02 8:45 UTC (permalink / raw) To: jpoimboe; +Cc: live-patching Hello guys, I'm new to livepatch world and now I'm a little confused with livepatch's per-task consistency model which is introduced by [1]. I've also readed the discussion on mailing list [2], which introduces shadow variable to handle data layout and semantic changes. But there's still some confusion with this per-task consistency model. According to the model, there will be scenario where old function and new function can co-exist, though for a single thread, it sees either all new functions or all old functions. I can't understand why Vojtech said that 'old func processing new data' was impossible. Assuming a scenario where a process calls func-A to submit a work request (inserted into a global list), and then a kthread is responsible for calling func-B to process all work requests in the list. What if this process has finished the transition (sees new func-A) while kthread still sees the old func-B? In this case, old func-B has to process new data. If there's some lock semantic changes in func-A and func-B, then old func-B has no way identifying the shadow variable labeled by new func-A. Please tell me if I missed something, and any suggestions will be appreciated. ;) Thanks. [1] livepatch: change to a per-task consistency model (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.6-rc3&id=d83a7cb375eec21f04c83542395d08b2f6641da2) [2] https://lkml.kernel.org/r/20141107140458.GA21774@suse.cz ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [help] Confusion on livepatch's per-task consistency model 2020-03-02 8:45 [help] Confusion on livepatch's per-task consistency model JeffleXu @ 2020-03-02 10:12 ` Petr Mladek 2020-03-02 12:56 ` JeffleXu 2020-03-02 10:36 ` Nicolai Stange 1 sibling, 1 reply; 6+ messages in thread From: Petr Mladek @ 2020-03-02 10:12 UTC (permalink / raw) To: JeffleXu; +Cc: jpoimboe, live-patching On Mon 2020-03-02 16:45:24, JeffleXu wrote: > Hello guys, > > > I'm new to livepatch world and now I'm a little confused with livepatch's > > per-task consistency model which is introduced by [1]. I've also readed the > > discussion on mailing list [2], which introduces shadow variable to handle > > data layout and semantic changes. But there's still some confusion with this > > per-task consistency model. > > > According to the model, there will be scenario where old function and new > > function can co-exist, though for a single thread, it sees either all new > > functions or all old functions. > > > I can't understand why Vojtech said that 'old func processing new data' was > > impossible. Assuming a scenario where a process calls func-A to submit a > > work request (inserted into a global list), and then a kthread is > responsible > > for calling func-B to process all work requests in the list. What if this > process > > has finished the transition (sees new func-A) while kthread still sees the > old func-B? > > In this case, old func-B has to process new data. If there's some lock > semantic > > changes in func-A and func-B, then old func-B has no way identifying the > shadow > > variable labeled by new func-A. > > > Please tell me if I missed something, and any suggestions will be > appreciated. ;) No, you did not miss anything. The consistency is only per-thread, If a livepatch is changing semantic of a global variable it must allow doing the changes only when the entire system is using the new code. And the new code must be able to deal with both old and new data. The new data semantic can be enable by post-patch callback that is called when the transition has finished. Fortunately, semantic changes are very rare at least in security fixes. Best Regards, Petr ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [help] Confusion on livepatch's per-task consistency model 2020-03-02 10:12 ` Petr Mladek @ 2020-03-02 12:56 ` JeffleXu 0 siblings, 0 replies; 6+ messages in thread From: JeffleXu @ 2020-03-02 12:56 UTC (permalink / raw) To: Petr Mladek; +Cc: jpoimboe, live-patching On 3/2/20 6:12 PM, Petr Mladek wrote: > On Mon 2020-03-02 16:45:24, JeffleXu wrote: >> Hello guys, >> >> >> I'm new to livepatch world and now I'm a little confused with livepatch's >> >> per-task consistency model which is introduced by [1]. I've also readed the >> >> discussion on mailing list [2], which introduces shadow variable to handle >> >> data layout and semantic changes. But there's still some confusion with this >> >> per-task consistency model. >> >> >> According to the model, there will be scenario where old function and new >> >> function can co-exist, though for a single thread, it sees either all new >> >> functions or all old functions. >> >> >> I can't understand why Vojtech said that 'old func processing new data' was >> >> impossible. Assuming a scenario where a process calls func-A to submit a >> >> work request (inserted into a global list), and then a kthread is >> responsible >> >> for calling func-B to process all work requests in the list. What if this >> process >> >> has finished the transition (sees new func-A) while kthread still sees the >> old func-B? >> >> In this case, old func-B has to process new data. If there's some lock >> semantic >> >> changes in func-A and func-B, then old func-B has no way identifying the >> shadow >> >> variable labeled by new func-A. >> >> >> Please tell me if I missed something, and any suggestions will be >> appreciated. ;) > No, you did not miss anything. The consistency is only per-thread, > If a livepatch is changing semantic of a global variable it must > allow doing the changes only when the entire system is using > the new code. And the new code must be able to deal with both > old and new data. > > The new data semantic can be enable by post-patch callback > that is called when the transition has finished. Thanks for replying. I didn't consider callback earlier, and yes it works in this case. Thanks. Jeffle ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [help] Confusion on livepatch's per-task consistency model 2020-03-02 8:45 [help] Confusion on livepatch's per-task consistency model JeffleXu 2020-03-02 10:12 ` Petr Mladek @ 2020-03-02 10:36 ` Nicolai Stange 2020-03-02 13:19 ` JeffleXu 1 sibling, 1 reply; 6+ messages in thread From: Nicolai Stange @ 2020-03-02 10:36 UTC (permalink / raw) To: JeffleXu; +Cc: jpoimboe, live-patching Hi, JeffleXu <jefflexu@linux.alibaba.com> writes: > According to the model, there will be scenario where old function and new > > function can co-exist, though for a single thread, it sees either all new > > functions or all old functions. That's correct. > I can't understand why Vojtech said that 'old func processing new > data' was impossible. Just to make it explicit: Vojtech was talking about data layout changes. That is, consider you have something like e.g. this in the unmodified kernel sources: struct my_driver_work { struct work_struct work; struct list_head works_list; void *some_payload; }; In general, you can't change that struct definition from a live patch. So simply extending it like this struct my_driver_work { struct work_struct work; struct list_head works_list; unsigned long flags /* New */ void *some_payload; }; won't work. > Assuming a scenario where a process calls func-A to submit a > > work request (inserted into a global list), and then a kthread is > responsible > > for calling func-B to process all work requests in the list. What if > this process > > has finished the transition (sees new func-A) while kthread still sees > the old func-B? Going with the example from above, the patched func-A would submit instances of the new struct my_driver_work whereas the unpatched func-B would expect the ->some_payload pointer at where ->flags is stored now, which is bad, obviously. In this specific example, it could perhaps be possible to make the patched func-A associate a shadow variable corresponding to the new ->flags member with newly created struct my_driver_work instances (of original, unmodified layout). Any unpatched func-B would obviously ignore it and consider the shadow only when it becomes patched. It very much depends on the specific situation whether or not this is acceptable. If not, the ->post_patch() can sometimes be used to achieve some notion of a "global consistency" state (in this context, c.f. Documentation/livepatch/system-state.rst). Note however, that the patched func-B must always be able to handle the situation where a struct my_driver_work instance does not have such a ->flags shadow attached to it, either because the instance had been created when the live patch has not been applied at all or because it has been submitted from a not yet transitioned func-A. There's another subtlety: the deallocation code for struct my_driver_work needs to get livepatched as well to make it free the ->flags shadow. Consider the case where func-A has been transitioned, but the deallocation code hasn't yet. Without any extra measures in func-A, it could happen that a stale ->flags shadow from a deallocated struct my_driver_work gets (wrongly) reassociated with a fresh struct my_driver_work instance allocated at the same address as the old one (because shadow variables are keyed on addresses of the data they're associated with). Sometimes that's acceptable, sometimes it's not. In the latter case you probably had to check for this situation and work around it in the allocation code, i.e. the live-patched func-A. Finally, let me remark that from my experience, most CVEs (>95%) can be fixed via live patching without having to resort to either of shadow variables, callbacks or the state API. For the rest, things usually tend to become really non-trivial, hackish and subtle. > In this case, old func-B has to process new data. If there's some lock > semantic > > changes in func-A and func-B, then old func-B has no way identifying > the shadow > > variable labeled by new func-A. I don't understand what you mean by "variable labeled by new func-A"? Anyway, it's correct that an unpatched func-B would not consider any shadow variables instantiated by patched func-A. And it's also correct that changing locking semantics is difficult, if not impossible. Thanks, Nicolai -- SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg), GF: Felix Imendörffer ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [help] Confusion on livepatch's per-task consistency model 2020-03-02 10:36 ` Nicolai Stange @ 2020-03-02 13:19 ` JeffleXu 2020-03-02 16:58 ` Nicolai Stange 0 siblings, 1 reply; 6+ messages in thread From: JeffleXu @ 2020-03-02 13:19 UTC (permalink / raw) To: Nicolai Stange; +Cc: jpoimboe, live-patching Thanks for replying. ;) On 3/2/20 6:36 PM, Nicolai Stange wrote: > Hi, > > JeffleXu <jefflexu@linux.alibaba.com> writes: > >> According to the model, there will be scenario where old function and new >> >> function can co-exist, though for a single thread, it sees either all new >> >> functions or all old functions. > That's correct. > > >> I can't understand why Vojtech said that 'old func processing new >> data' was impossible. > Just to make it explicit: Vojtech was talking about data layout > changes. Fine > > That is, consider you have something like e.g. this in the unmodified > kernel sources: > > struct my_driver_work > { > struct work_struct work; > struct list_head works_list; > void *some_payload; > }; > > In general, you can't change that struct definition from a live > patch. So simply extending it like this > > struct my_driver_work > { > struct work_struct work; > struct list_head works_list; > unsigned long flags /* New */ > void *some_payload; > }; > > won't work. > > >> Assuming a scenario where a process calls func-A to submit a >> >> work request (inserted into a global list), and then a kthread is >> responsible >> >> for calling func-B to process all work requests in the list. What if >> this process >> >> has finished the transition (sees new func-A) while kthread still sees >> the old func-B? > Going with the example from above, the patched func-A would submit > instances of the new struct my_driver_work whereas the unpatched func-B > would expect the ->some_payload pointer at where ->flags is stored now, > which is bad, obviously. > > In this specific example, it could perhaps be possible to make the > patched func-A associate a shadow variable corresponding to the new > ->flags member with newly created struct my_driver_work instances (of > original, unmodified layout). Any unpatched func-B would obviously > ignore it and consider the shadow only when it becomes patched. It very > much depends on the specific situation whether or not this is > acceptable. If not, the ->post_patch() can sometimes be used to achieve > some notion of a "global consistency" state (in this context, > c.f. Documentation/livepatch/system-state.rst). Well, I'm familiar with shadow variable, but didn't consider callbacks earlier. Since the version of my kernel is not new enough, the "system state API" has not been merged in my kernel. I will read it later. > Note however, that the patched func-B must always be able to handle > the situation where a struct my_driver_work instance does not have such > a ->flags shadow attached to it, either because the instance had been > created when the live patch has not been applied at all or because it > has been submitted from a not yet transitioned func-A. > > There's another subtlety: the deallocation code for struct > my_driver_work needs to get livepatched as well to make it free the > ->flags shadow. Consider the case where func-A has been transitioned, > but the deallocation code hasn't yet. Without any extra measures in > func-A, it could happen that a stale ->flags shadow from a deallocated > struct my_driver_work gets (wrongly) reassociated with a fresh struct > my_driver_work instance allocated at the same address as the old one > (because shadow variables are keyed on addresses of the data they're > associated with). Sometimes that's acceptable, sometimes it's not. In > the latter case you probably had to check for this situation and work > around it in the allocation code, i.e. the live-patched func-A. I've never thought about this. It's a valuable suggestion. > Finally, let me remark that from my experience, most CVEs (>95%) can be > fixed via live patching without having to resort to either of shadow > variables, callbacks or the state API. For the rest, things usually tend > to become really non-trivial, hackish and subtle. Thanks for your experience. >> In this case, old func-B has to process new data. If there's some lock >> semantic >> >> changes in func-A and func-B, then old func-B has no way identifying >> the shadow >> >> variable labeled by new func-A. > I don't understand what you mean by "variable labeled by new func-A"? > Anyway, it's correct that an unpatched func-B would not consider any > shadow variables instantiated by patched func-A. And it's also correct > that changing locking semantics is difficult, if not impossible. Just means shadow variable allocated by new patched function. I know shadow variable can serve as a flag to enable the new functions, but in this case the post_patch() callback is obviously more appropriate to serve as this role. So as far as I understand, for all kinds of (data/locking) semantic changes, it's the responsibility of the patch writer to detect the semantic changes, and usually it can only be analyzed case by case. Right? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [help] Confusion on livepatch's per-task consistency model 2020-03-02 13:19 ` JeffleXu @ 2020-03-02 16:58 ` Nicolai Stange 0 siblings, 0 replies; 6+ messages in thread From: Nicolai Stange @ 2020-03-02 16:58 UTC (permalink / raw) To: JeffleXu; +Cc: Nicolai Stange, jpoimboe, live-patching JeffleXu <jefflexu@linux.alibaba.com> writes: > So as far as I understand, for all kinds of (data/locking) semantic > changes, it's the responsibility of the patch writer to detect the > semantic changes, and usually it can only be analyzed case by > case. Right? Exactly :) Nicolai -- SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg), GF: Felix Imendörffer ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-02 16:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-02 8:45 [help] Confusion on livepatch's per-task consistency model JeffleXu 2020-03-02 10:12 ` Petr Mladek 2020-03-02 12:56 ` JeffleXu 2020-03-02 10:36 ` Nicolai Stange 2020-03-02 13:19 ` JeffleXu 2020-03-02 16:58 ` Nicolai Stange
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).