On Fri, Mar 21, 2025 at 11:27:34AM -0400, Liam R. Howlett wrote: > +cc Peter due to uffd interests Gentle nudge for Peter to make himself uffd maintainer :) I am not a fan of this 'happen to know person X often touches Y' stuff, this is what MAINTAINERS is for. If you're not there, good chance I won't cc you... I also strongly feel we need somebody to take overall responsibility for uffd at this point. Pints will be bought for this person in Montreal ;) > > * Pedro Falcato <pfalcato@xxxxxxx> [250321 07:26]: > > On Fri, Mar 21, 2025 at 10:09:37AM +0000, Lorenzo Stoakes wrote: > > > Currently, if a VMA merge fails due to an OOM condition arising on commit > > > merge or a failure to duplicate anon_vma's, we report this so the caller > > > can handle it. > > > > > > However there are cases where the caller is only ostensibly trying a > > > merge, and doesn't mind if it fails due to this condition. > > > > > > > Ok, so here's my problem with your idea: I don't think merge should be exposed > > to vma_modify() callers. Right now (at least AIUI), you want to modify a given > > VMA, you call vma_modify(), and it gives you a vma you can straight up modify > > without any problems. Essentially breaks down any VMAs necessary. This feels > > contractually simple and easy to use, and I don't think leaking details about > > merging is the correct approach here. > > > > > Since we do not want to introduce an implicit assumption that we only > > > actually modify VMAs after OOM conditions might arise, add a 'give up on > > > oom' option and make an explicit contract that, should this flag be set, we > > > absolutely will not modify any VMAs should OOM arise and just bail out. > > > > > > > Thus, to me the most natural solution is still mine. Do you think it places too > > many constraints on vma_modify()? vma_modify() on a single VMA, without > > splitting, Just Working(tm) is a sensible expectation (and vma_merge being fully > > best-effort). Things like mprotect() failing due to OOM are also pretty disastrous, > > so if we could limit that it'd be great. > > > > In any case, your solution looks palatable to me, but I want to make > > sure we're not making this excessively complicated. > > Either solution is fine with me, but... > Thanks. > I hate both of them, and I (mostly) blame uffd. Some blame is on syzbot > for exposing me to this unreachable failure. ;-) So do I. But as I said to Pedro, we -must- express this contract that we won't remove VMAs before returning, otherwise later somebody might change logic which changes this, not realise they just broke these stupid horrible edge cases (that 99.9999-recurring-9% will never happen in reality) and that's just not something I want to happen. > > I think Lorenzo's patch points out that we should have a better way to > deal with a return and a vma pointer and we basically have that a lot of > other places with the structures that we thread through to deal with > mostly unreachable errors that syzbot injects. I dislike flags passed > in to treat things differently. We are adding an 8th argument to the > function (of type boolean) to work around this.. The top of this make sa very good point, but in reference to params, flags, etc... well I have to say something :) Yeah but it's a convenience wrapper function, literally build explicitly for uffd. I think it's not quite as terrible a situation as you think. And it seems to me having a flag/mode to explicitly request something is fine? Was it more palatable as a bit field prior to Vlastimil's request to move to bit fields?... Because you must object to a lot of the kernel if you dislike that kind of thing :) A lot of the point of having threaded state is that we can modify things for specific cases. > > I think Pedro's patch is working around the same issue of return value > vs return pointers. Other places we pass in &prev and just do what we > need to in the caller and just check the return value - which I also > hate, especially when you look at the assembly generated to deal with a > "non-problem" problem. Let me group a reply to all of these as you're making a very good point that I think has a deeper solution... > > I have no better solution and need to spend more time looking into it, > but the number of times we have syzbot failing a random allocation and > finding issues.. well, it's basically a class of bugs we handle very > poorly throughout our stack. > > Realistically, I *hope* that Lorenzo's additional argument will make > code authors think twice about the failure scenario when calling the > function. > > Pedro's code fixes this caller, but leaves the possibility of someone > missing this again in the future, I think? This could be made more > obvious with some clever naming of functions, perhaps try_<something>? Renaming like that will not affect changes to the core merge/functions called by merge. We need that explicit contract. So... > > We are essentially avoiding the compiler from catching the error for us > by returning that ERR_PTR(), which (keeping with the theme of my email) > I hate. It's fine for little functions but we've made a mess of it too > often. > > Reality will probably not align with the realistic view and people will > just copy/paste from wherever they saw it called... so we should think > twice about the failure scenarios on code review and I think a flag > (or a function name change?) might make this more obvious. OK so what I think we have have is a break in abstraction, where we are trying to do an 'iteration through VMAs where it's convenient to keep track of prev' and then people duplicating the code, making subtly false assumptions (yes pointer being returned with the obnoxious ERR_PTR() stuff possible and -god knows what happens to the state if not present-) and etc. etc. Don't we just need a new kind of vma iterator that handles the prev bits for us that can just do away with this crap? How we get it to interact with everything else is left as a fun one for thinking about in hot baths, aeroplanes or at trampoline parks (it'll be me who ends up thinking about his won't it?) Anyway it's all horrid, but I still think (in, of course, an entirely unbiased fashion) my solution is the way forward, but I agree with and Pedro that it's vom-inducing yes. > > Thanks, > Liam
