On Wed, Sep 25, 2024 at 07:14:51PM GMT, Pedro Falcato wrote: > On Wed, Sep 25, 2024 at 06:04:59PM GMT, Lorenzo Stoakes wrote: > > On Wed, Sep 25, 2024 at 09:19:17AM GMT, Shakeel Butt wrote: > > > I have no idea what makes you think I am blocking the feature that you > > > repond in a weird tone but let me be upfront what I am asking: Let's > > > collectively decide which is the better option (in terms of > > > maintainability and extensibility) and move forward. > > > > I'm not sure what you mean by 'weird tone'... perhaps a miscommunication? > > > > To summarise in my view - a suggestion was made to, rather than provide the > > proposed flag - a pidfd sentinel should be introduced. > > > > Simply introducing a sentinel that represents 'the current process' without > > changing interfaces that accept a pidfd would be broken - so implementing > > this implies that _all_ pidfd interfaces are updated, as well as tests. > > > > While I suggested PIDFD_SELF, I never meant that we should change every interface, > but rather adopt a sound, consistent strategy for pidfd interfaces and stick with > it for the foreseeable future. If we add this to a public uapi-facing header and document it as 'refer to self', we will introduce something that users will receive and be confused if they are unable to use anywhere else. This is the fundamental problem with this. It's a fundamental, permanent uAPI change which either introduces confusion - because it only works for process_madvise() - or will need work along with test updates etc. And it is very much out of scope for this series as a result. I may just back out of doing this and replace this with something simpler that causes less push-back (and fixes existing broken behaviour in process_madvise()) that lets me do what I need to do with the guard pages. I don't think anybody can object to a self-pidfd having unrestricted access to madvise flags... I may in parallel just try to implement the pidfd sentinel idea and let it take that long time and update process_madvise() later, as I can't really let this block the guard page work. > > In this case, we'd adapt process_madvise, then possibly later pidfd_send_signal, etc. > There are plenty of pidfd interfaces that don't make sense with a PIDFD_SELF. Various > other interfaces will probably never want to adopt it at all (select _can't_, other > fs syscalls such as read/write/poll/whatever would require awful handholding from > various kernel subsystems, while in that sense we would definitely require a proper > struct file/inode/whatever for each pseudo-fd, which is not exactly what we want). > And arguably, you'd have to audit all of them and decide. I mean I think this kind of sums up my point really right? Again, I don't object to the idea, I object to the suggestion that you don't need to do this other work. > > I suggest doing so is, of course, entirely out of the scope of this > > change. Therefore if we were to require that here - it would block the > > feature while I go work on that. > > > > I think this is pretty clear right? And I also suggest that doing so is > > likely to take quite some time, and may not even have a positive outcome. > > > > So it's not a case of 'shall we take approach A or approach B?' but rather > > 'should we take approach A or entirely implement a new feature B, then once > > that is done, use it'. > > > > So as to your 'collectively decide what is the better option' - in my > > previous response I argued that the best approach between 'use an > > unimplemented suggested entirely new feature of pidfd' vs. 'implement a > > flag that would in no way block the prior approach' - a flag works better. > > I just don't think it's a new feature, just an established, future-proof way > of doing things :) Your patch should remain mostly similar apart from switching > the flag check into an fd check. It is absolutely a new feature, you're adding an entirely new UAPI-visible flag that is applicable to all pidfd's, unless we make it process_madvise()-specific in the flag, and I'm not sure that's going to get accepted. It also needs to be separately accepted by the maintainers of the relevant file header etc. > > > > > > > By big undertaking, do you mean other syscalls that take pidfd > > > (pidfd_getfd, pidfd_send_signal & process_mrelease) to handle PIDFD_SELF > > > or something else? > > > > I mean if you add a pidfd sentinel that represents 'the current process' it > > may get passed to any interface that accepts a pidfd, so all of them have > > to handle it _somehow_. > > > > Also you'll want to update tests accordingly and clearly need to get > > community buy-in for that feature. > > > > You may want to just add a bunch of: > > > > if (pidfd == SENTINEL) > > return -EINVAL; > > It should already be there in the form of an -EBADF. :) this is pseudo-code. And I'd want to check all pidfd handled it correctly. I mean you'd think so right... > > > > > So it's not impossible my instincts are off and we can get away with simply > > doing that. > > > > On the other hand, would that be confusing? Wouldn't we need to update > > documentation, manpages, etc. to say explicitly 'hey this sentinel is just > > not supported'? > > This is a fair point, but we could also just... not :) which I don't feel is too > wrong, since the fd works kind of like a flag here. Yeah, no, I think you would have to. It's not specific to process_madvise(), it's a general fd flag. And like I said, if we did introduce this we'd need additional assessment and review from those guys. > > -- > Pedro
