Re: pg_jobc going negative?
On Fri, Jul 10, 2020 at 08:41:40PM +0700, Robert Elz wrote: > I have spent a little time looking at this now, and I think > it is just all a mess. Indeed... > As best I can work out, and someone correct me if I'm wrong, > the whole purpose of pg_jobc is so that orphanpg() can be called > when a process group is orphaned (no longer has a session leader). Yes. A process group is orphaned if there's no longer a way to do job control on it directly -- the most common case is when you get hung up on and your login shell/session leader goes away, but it can also happen if you make chains of forks and some of them exit. For example, if you start an interactive subshell, run some stuff in the background, then exit it, the background jobs are now orphaned. Detecting orphaned process groups is vexing because it's a graph connectivity problem and you don't want to/can't (for locking reasons) go chugging around the graph explicitly. > I see 3 ways forward... simply drop the KASSERT the way that FreeBSD > have done, and let things return to the semi-broken but rarely bothersome > code that was there before. That's not really a good idea, as the > sanitizers apparently find problems with the code the way it was (not > too surprising, deleting the KASSERT won't fix the bugs, it just stops > noticing them explicitly). Indeed. > Or, we could properly define what pg_jobc is counting, and then make sure > that it counts whatever that is properly - is incremented in all the > appropriate places, and decremented properly as well. Currently > the comment about it in proc.h is: > /* > * Number of processes qualifying > * pgrp for job control > */ > which makes it clear that it is a reference counter (not necessarily > counting the number of something which exists, so that something can be > deleted, but it is counting references to processes). Unfortunately > I have no idea what "qualifying pgrp for job control" is supposed to mean. A processes is job-controlled by its parent process, and a process group is job-controlled by any parent of processes in it. So a process group can be job-controlled if some process in it has a parent in a different process group that can also be job-controlled, going back to the session leader. (Provided that the session has a tty, anyway.) This becomes complicated because although normal process group usage is completely hierarchical, it's possible to place processes in process groups such that the graph of parent relationships ceases to be a tree. The code is trying to count how many processes in the process group have a parent in a different process group that is not itself orphaned. If this count reaches zero, the process group has become orphaned. I can't remember if it's possible to set up process groups where the parent graph is cyclic. I think it might be; or at least, I don't remember any reason why not that wouldn't also force the graph to be a tree. But it's been a while with all this stuff and I may be forgetting something. Anyway, the method should work as long as the graph remains acyclic. The implementation is probably missing cases where it should be adjusting the count. > That could be done, but it seems like a lot of work to me, and not easy. Unfortunately, I think it's the only viable way forard. > Another (more radical) approach would be to simply drop orphanpg() > completely, and thus no longer need pg_jobc at all. The system > wouldn't be bothered by this at all - all orphanpg() does is locate > stopped members of the process group, and send then SIGCONT (to restart) > and SIGHUP (to probably kill them - or at least inform them they their > environment has changed). Unfortunately, removing it violates POSIX, which prescribes all this stuff. > Third, and the option I'd suggest, is to revert to more basic principles, > remove the pg_jobc attempt to detect when a session leader has exited, > or changed to a different process group, and instead at candidate events > (any time a process leaves a process group, for any reason) check if that > process was the session leader, and if it is, clean up any now orphaned > stopped processes. This is likely to be slower than the current attempt, > but is much easier to make correct (and much less likely to cause problems, > other than dangling orphaned stopped processes, if incorrect). I don't think that's sufficient, because it doesn't take into account the cases where jobs become disconnected from the session leader without the session leader exiting. Which is easy to arrange: su, then vi, ^Z, exit. :-/ -- David A. Holland dholl...@netbsd.org
Re: pg_jobc going negative?
>> [...] I'd be astonished if there aren't at least a few programs >> [in pkgsrc] that grub around in things like this. > No question, there are, but this particular field seems very unlikely > to have any users - really really unlikely. Especially since, come to think of it, the value is incorrect often enough for those asserts to fire, so anything depending on it is already broken. Good point. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: pg_jobc going negative?
In article <27763.1594388...@jinx.noi.kre.to>, Robert Elz wrote: >Date:Tue, 9 Jun 2020 08:23:19 - (UTC) >From:mlel...@serpens.de (Michael van Elst) >Message-ID: > >I have spent a little time looking at this now, and I think >it is just all a mess. > > | pg_jobc is not a reference counter. > >Maybe not technically a "reference" counter, as what it is counting isn't >strictly references, but anything that has x++ and if (--x == 0) do_stuff() >is essentially a reference counter. What it is counting references to >isn't clear (particularly here), but that is what it is doing, or trying >to do (it has all the same issues as things which really are ref counters). > > | The assertion probably stopped > | a bug in a different place by coincidence. > >I doubt that, this code is not at all good. There is no question but >that the counter does not count properly. > >As best I can work out, and someone correct me if I'm wrong, >the whole purpose of pg_jobc is so that orphanpg() can be called >when a process group is orphaned (no longer has a session leader). > >If it has any other use, I cannot see it. > >What's there now simply doesn't work for this purpose. It was >suggested that the FreeBSD code has been modified from what we >have, and that simply adopting that might work. I went to look >at their code, but before I did that, I saw that a month ago >(that is, just around the time of the original discussion here) >they copied maxv's KASSERTs into their code. A week ago they removed >them again, as they were occasionally firing. That tells me, >even without looking at their code, that they (still) have similar >bugs to what we do, and thus that just importing their code won't >help us. > >I see 3 ways forward... simply drop the KASSERT the way that FreeBSD >have done, and let things return to the semi-broken but rarely bothersome >code that was there before. That's not really a good idea, as the >sanitizers apparently find problems with the code the way it was (not >too surprising, deleting the KASSERT won't fix the bugs, it just stops >noticing them explicitly). > >Or, we could properly define what pg_jobc is counting, and then make sure >that it counts whatever that is properly - is incremented in all the >appropriate places, and decremented properly as well. Currently >the comment about it in proc.h is: >/* > * Number of processes qualifying >* pgrp for job control > */ >which makes it clear that it is a reference counter (not necessarily >counting the number of something which exists, so that something can be >deleted, but it is counting references to processes). Unfortunately >I have no idea what "qualifying pgrp for job control" is supposed to mean. > >That could be done, but it seems like a lot of work to me, and not easy. > >Another (more radical) approach would be to simply drop orphanpg() >completely, and thus no longer need pg_jobc at all. The system >wouldn't be bothered by this at all - all orphanpg() does is locate >stopped members of the process group, and send then SIGCONT (to restart) >and SIGHUP (to probably kill them - or at least inform them they their >environment has changed). If the system wasn't doing this, users manually >(or a script run from cron or something) could do it instead, If not done >at all, badly behaving session leaders (processes which don't clean up >their stopped jobs before exiting - including ones with bugs that causes >them to abort) would over time cause a growing number of stopped jobs to >simply clutter the system, consuming various resources, but otherwise >harmless (there is nothing special about the processes, they can be killed, >or continued - it is just that the process which would normally do that >is no longer around). > >Third, and the option I'd suggest, is to revert to more basic principles, >remove the pg_jobc attempt to detect when a session leader has exited, >or changed to a different process group, and instead at candidate events >(any time a process leaves a process group, for any reason) check if that >process was the session leader, and if it is, clean up any now orphaned >stopped processes. This is likely to be slower than the current attempt, >but is much easier to make correct (and much less likely to cause problems, >other than dangling orphaned stopped processes, if incorrect). > >As best I can tell, all the data needed exists already, all that will be >needed is to modify the code. We can even leave pg_jobc in the pgrp >struct, to avoid needing a kernel version bump (and for reasons I cannot >imagine, pg_jobc is copied into kinfo and einfo structs for sysctl and /proc >access to the process data, so leaving it around avoids needing to version >those interfaces as well ... the value would be a meaningless 0, always, but >I really find it hard to believe that anything would ever care, or even >notice). > >Opinions on any of this before I start
Re: pg_jobc going negative?
Date:Fri, 10 Jul 2020 16:47:28 +0200 From:Rhialto Message-ID: <20200710144728.gy3...@falu.nl> | It also seems to be involved in deciding wether to send a SIGTTOU or | SIGTTIN to a process Ah, right, thanks ... when I was reviewing uses in the kernel I was concentrating on places where pg_jobc changes, and just sort of dismissed the places where it was simply examined then I never went back to them again. I will make sure my plans keep this working (I knew that orphaned pg processes no longer do any kind of auto-stop, though they can stil be sent SIGSTOP I believe). | I found this above fixjobc() which goes into a bit more detail what is | being counted: Yes, I've been looking at that - that's where things are going wrong I think, when looking at the children each child can decrement pg_jobc but it only seems to get incremented once. Easy to see how it becomes negative. mo...@rodents-montreal.org said: | But each of those steps involves some winnowing-down. Yes, again speculating, but my guess would be (for libkvm) "if someone uses it, we make it available, if no-one does, we don't" and quite probably ps was using it via /dev/mem already (though what use that was supposed to have I cannot guess, perhaps just for debugging the kernel implementation) After that it appears that everything was just copied to each new interface, based upon the philosophy of phasing out uses of libkvm, which would mean making sure that the alternate interface could do everything libkvm could do, otherwise someone would find the missing data a justification for sticking with libkvm. mo...@rodents-montreal.org said: | Did your userland sweep include pkgsrc? No, I don't have the resources to do that. I don't use many packages (so don't have many distfiles) and have even fewer of those unpacked. My test system for HEAD has none of it at all. mo...@rodents-montreal.org said: | [...] I'd be astonished if there aren't at least a few | programs there that grub around in things like this. No question, there are, but this particular field seems very unlikely to have any users - really really unlikely. kre
Re: pg_jobc going negative?
On Fri 10 Jul 2020 at 20:41:40 +0700, Robert Elz wrote: > As best I can work out, and someone correct me if I'm wrong, > the whole purpose of pg_jobc is so that orphanpg() can be called > when a process group is orphaned (no longer has a session leader). > > If it has any other use, I cannot see it. It also seems to be involved in deciding wether to send a SIGTTOU or SIGTTIN to a process (i.e., to suspend a process if it tries to read from the tty, but it is in the background). In sys/kern/tty.c and sys/kern/tty_pty.c. > Or, we could properly define what pg_jobc is counting, and then make sure > that it counts whatever that is properly - is incremented in all the > appropriate places, and decremented properly as well. Currently > the comment about it in proc.h is: > /* > * Number of processes qualifying >* pgrp for job control > */ > which makes it clear that it is a reference counter (not necessarily > counting the number of something which exists, so that something can be > deleted, but it is counting references to processes). Unfortunately > I have no idea what "qualifying pgrp for job control" is supposed to mean. I found this above fixjobc() which goes into a bit more detail what is being counted: /* * Adjust pgrp jobc counters when specified process changes process group. * We count the number of processes in each process group that "qualify" * the group for terminal job control (those with a parent in a different * process group of the same session). If that count reaches zero, the * process group becomes orphaned. Check both the specified process' * process group and that of its children. -Olaf. -- Olaf 'Rhialto' Seibert -- rhialto at falu dot nl ___ Anyone who is capable of getting themselves made President should on \X/ no account be allowed to do the job. --Douglas Adams, "THGTTG" signature.asc Description: PGP signature
Re: pg_jobc going negative?
On 10.07.2020 15:41, Robert Elz wrote: > Unfortunately > I have no idea what "qualifying pgrp for job control" is supposed to mean. The Design and implementation of 4.4book phrases it as: number of processes with parent controlling terminal. Unfortunately the book does not explain whether pg_jobc can go bellow 0. signature.asc Description: OpenPGP digital signature
Re: pg_jobc going negative?
> [... pg_jobc ...] > I see 3 ways forward... I count 4, but maybe kre is counting two of them as subclasses of a single one. > simply drop the KASSERT the way that FreeBSD have done, and let > things return to the semi-broken but rarely bothersome code that was > there before. > Or, we could properly define what pg_jobc is counting, and then make > sure that it counts whatever that is properly [...] > Another (more radical) approach would be to simply drop orphanpg() > completely, and thus no longer need pg_jobc at all. > Third, and the option I'd suggest, is to revert to more basic > principles, remove the pg_jobc attempt to detect when a session > leader has exited, or changed to a different process group, and > instead at candidate events (any time a process leaves a process > group, for any reason) check if that process was the session leader, > and if it is, clean up any now orphaned stopped processes. Not surprisingly in view of who put it forward, I would agree with this suggestion. But that alone isn't worth an email. The main thing prompting me to write this mail is > We can even leave pg_jobc in the pgrp struct, to avoid needing a > kernel version bump (and for reasons I cannot imagine, pg_jobc is > copied into kinfo and einfo structs for sysctl and /proc access to > the process data, so leaving it around avoids needing to version > those interfaces as well ... the value would be a meaningless 0, > always, but I really find it hard to believe that anything would ever > care, or even notice). It seems to me that if pg_jobc is exported, someone presumably once cared and there's thus a decent chance someone still cares. Did you do a sweep for userland references to it? It seems plausible to me that it's used, at least for zero/nonzero, by userland tools that are interested in process groups. Is there any record of who added the code to export pg_jobc to userland? If so, and if that person is still around, it might be worth sending a question thataway to see if any explanation might be forthcoming. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
Re: pg_jobc going negative?
Date:Tue, 9 Jun 2020 08:23:19 - (UTC) From:mlel...@serpens.de (Michael van Elst) Message-ID: I have spent a little time looking at this now, and I think it is just all a mess. | pg_jobc is not a reference counter. Maybe not technically a "reference" counter, as what it is counting isn't strictly references, but anything that has x++ and if (--x == 0) do_stuff() is essentially a reference counter. What it is counting references to isn't clear (particularly here), but that is what it is doing, or trying to do (it has all the same issues as things which really are ref counters). | The assertion probably stopped | a bug in a different place by coincidence. I doubt that, this code is not at all good. There is no question but that the counter does not count properly. As best I can work out, and someone correct me if I'm wrong, the whole purpose of pg_jobc is so that orphanpg() can be called when a process group is orphaned (no longer has a session leader). If it has any other use, I cannot see it. What's there now simply doesn't work for this purpose. It was suggested that the FreeBSD code has been modified from what we have, and that simply adopting that might work. I went to look at their code, but before I did that, I saw that a month ago (that is, just around the time of the original discussion here) they copied maxv's KASSERTs into their code. A week ago they removed them again, as they were occasionally firing. That tells me, even without looking at their code, that they (still) have similar bugs to what we do, and thus that just importing their code won't help us. I see 3 ways forward... simply drop the KASSERT the way that FreeBSD have done, and let things return to the semi-broken but rarely bothersome code that was there before. That's not really a good idea, as the sanitizers apparently find problems with the code the way it was (not too surprising, deleting the KASSERT won't fix the bugs, it just stops noticing them explicitly). Or, we could properly define what pg_jobc is counting, and then make sure that it counts whatever that is properly - is incremented in all the appropriate places, and decremented properly as well. Currently the comment about it in proc.h is: /* * Number of processes qualifying * pgrp for job control */ which makes it clear that it is a reference counter (not necessarily counting the number of something which exists, so that something can be deleted, but it is counting references to processes). Unfortunately I have no idea what "qualifying pgrp for job control" is supposed to mean. That could be done, but it seems like a lot of work to me, and not easy. Another (more radical) approach would be to simply drop orphanpg() completely, and thus no longer need pg_jobc at all. The system wouldn't be bothered by this at all - all orphanpg() does is locate stopped members of the process group, and send then SIGCONT (to restart) and SIGHUP (to probably kill them - or at least inform them they their environment has changed). If the system wasn't doing this, users manually (or a script run from cron or something) could do it instead, If not done at all, badly behaving session leaders (processes which don't clean up their stopped jobs before exiting - including ones with bugs that causes them to abort) would over time cause a growing number of stopped jobs to simply clutter the system, consuming various resources, but otherwise harmless (there is nothing special about the processes, they can be killed, or continued - it is just that the process which would normally do that is no longer around). Third, and the option I'd suggest, is to revert to more basic principles, remove the pg_jobc attempt to detect when a session leader has exited, or changed to a different process group, and instead at candidate events (any time a process leaves a process group, for any reason) check if that process was the session leader, and if it is, clean up any now orphaned stopped processes. This is likely to be slower than the current attempt, but is much easier to make correct (and much less likely to cause problems, other than dangling orphaned stopped processes, if incorrect). As best I can tell, all the data needed exists already, all that will be needed is to modify the code. We can even leave pg_jobc in the pgrp struct, to avoid needing a kernel version bump (and for reasons I cannot imagine, pg_jobc is copied into kinfo and einfo structs for sysctl and /proc access to the process data, so leaving it around avoids needing to version those interfaces as well ... the value would be a meaningless 0, always, but I really find it hard to believe that anything would ever care, or even notice). Opinions on any of this before I start banging on the code? kre ps: I don't believe that any of the problems here are race conditions, the counter is simply not maintained correctly (which isn't to
Re: pg_jobc going negative?
Date:Mon, 29 Jun 2020 23:22:52 +0200 From:Kamil Rytarowski Message-ID: | Ping? This kernel crash is blocking GDB/etc and it is an instant crash. Sorry, been side-tracked, will get to it soon. kre
Re: pg_jobc going negative?
On 09.06.2020 20:11, Robert Elz wrote: > Date:Tue, 9 Jun 2020 17:04:54 +0200 > From:Kamil Rytarowski > Message-ID: > > > | Yes... syzkaller had like 12 different ways to reproduce it. > > OK, thanks. > > | There is still a race and we randomly go to negative pg_jobc. > > I am not at all surprised... > > I will look at it over the next couple of days. No guarantees... > Ping? This kernel crash is blocking GDB/etc and it is an instant crash. > kre >
Re: pg_jobc going negative?
Date:Tue, 9 Jun 2020 14:16:16 -0400 From:Christos Zoulas Message-ID: | The FreeBSD refactoring LGTM. It also simplifies the code. Sorry, been off net all day ... that may very well be the way to go, but I'd like to understand what is happening with our current code first, so we will know that the changes are fixing a problem, and not just moving things around. kre
Re: pg_jobc going negative?
The FreeBSD refactoring LGTM. It also simplifies the code. christos > On Jun 9, 2020, at 2:11 PM, Robert Elz wrote: > >Date:Tue, 9 Jun 2020 17:04:54 +0200 >From:Kamil Rytarowski >Message-ID: > > > | Yes... syzkaller had like 12 different ways to reproduce it. > > OK, thanks. > > | There is still a race and we randomly go to negative pg_jobc. > > I am not at all surprised... > > I will look at it over the next couple of days. No guarantees... > > kre signature.asc Description: Message signed with OpenPGP
Re: pg_jobc going negative?
Date:Tue, 9 Jun 2020 17:04:54 +0200 From:Kamil Rytarowski Message-ID: | Yes... syzkaller had like 12 different ways to reproduce it. OK, thanks. | There is still a race and we randomly go to negative pg_jobc. I am not at all surprised... I will look at it over the next couple of days. No guarantees... kre
Re: pg_jobc going negative?
On 09.06.2020 16:35, Robert Elz wrote: > Date:Tue, 9 Jun 2020 14:13:56 +0200 > From:Kamil Rytarowski > Message-ID: <85d5e51f-afd1-1038-fd68-2366ff073...@netbsd.org> > > | Here is the simplest reproducer crashing the kernel on negative pg_jobc: > > I have not looked at this closely yet, but this is likely because > ptrace() fiddles p_pptr which the routines that manipulate the pg_jobc > more or less expect to be a constant. > > Is there any known reproducer of this problem which does not involve ptrace() > ? > Yes... syzkaller had like 12 different ways to reproduce it. As far as I can tell, in the syzkaller case they all are about races. One of them is here: https://syzkaller.appspot.com/text?tag=ReproC=128060f610 After adding the asserts, all look similar to me: forking + setpgid(0, 0). > At first glance, the manipulations of pg_jobc looks a bit dodgy to me, but I > haven't investigated enough to be able to spot a definite problem yet > (possible ptrace() generated issue aside - and yes, those need to work as > well). > > I doubt very much that adding a new mutex will make a difference, all the > manipulations are done with proc_lock held, which is kind of the "big lock" > for process manipulation - adding finer grained locking might improve > performance, by improving concurrency, but is unlikely (at this stage, > nothing is impossible) to be a fix for this problem. > There is still a race and we randomly go to negative pg_jobc. > kre >
Re: pg_jobc going negative?
Date:Tue, 9 Jun 2020 14:13:56 +0200 From:Kamil Rytarowski Message-ID: <85d5e51f-afd1-1038-fd68-2366ff073...@netbsd.org> | Here is the simplest reproducer crashing the kernel on negative pg_jobc: I have not looked at this closely yet, but this is likely because ptrace() fiddles p_pptr which the routines that manipulate the pg_jobc more or less expect to be a constant. Is there any known reproducer of this problem which does not involve ptrace() ? At first glance, the manipulations of pg_jobc looks a bit dodgy to me, but I haven't investigated enough to be able to spot a definite problem yet (possible ptrace() generated issue aside - and yes, those need to work as well). I doubt very much that adding a new mutex will make a difference, all the manipulations are done with proc_lock held, which is kind of the "big lock" for process manipulation - adding finer grained locking might improve performance, by improving concurrency, but is unlikely (at this stage, nothing is impossible) to be a fix for this problem. kre
Re: pg_jobc going negative?
On 09.06.2020 08:38, Maxime Villard wrote: >> Should we allow pg_jobc going negative? > > I don't think so, the code is not designed to expect negative values. Here is the simplest reproducer crashing the kernel on negative pg_jobc: http://netbsd.org/~kamil/ptrace/pg_jobc-crash.c On 09.06.2020 08:38, Maxime Villard wrote: > In short, (1) my understanding of it is that the code is not designed to > expect negative values, and (2) since I added the KASSERTs 10/11 of the > random bugs didn't trigger. Big signs the bug is indeed related to > refcounting. > > It would be nice if someone with better understanding than me of the lwp > group stuff could have a look, though. Generally, pg_jobc looks like a ref counting mechanism. FreeBSD reworked the code and added a dedicated pgrp mutex. I don't know which path is the best for us, especially regarding the SMP properties. + ad@
Re: pg_jobc going negative?
On 09.06.2020 10:23, Michael van Elst wrote: > m...@m00nbsd.net (Maxime Villard) writes: > >> You can see they are all different, but all have to do with reading the >> group pointer, which was either freed, overwritten, not initialized, >> unmapped, or contained pure garbage. This is typical of refcounting bugs >> where a resource disappears under your feet. > > pg_jobc is not a reference counter. The assertion probably stopped > a bug in a different place by coincidence. > As the first step, is it fine to replace all pg_jobc == 0/ != 0 checks with pg_jobc > 0 / <= 0? This should not make anything worse than it is now. The remaining code assumes that pg_jobc never goes below 0. And then, follow up with the removal of the assert. We will check with syzkaller whether the races/crashes are gone. signature.asc Description: OpenPGP digital signature
Re: pg_jobc going negative?
m...@m00nbsd.net (Maxime Villard) writes: >You can see they are all different, but all have to do with reading the >group pointer, which was either freed, overwritten, not initialized, >unmapped, or contained pure garbage. This is typical of refcounting bugs >where a resource disappears under your feet. pg_jobc is not a reference counter. The assertion probably stopped a bug in a different place by coincidence. -- -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
Re: pg_jobc going negative?
mo...@rodents-montreal.org (Mouse) writes: >> pg_jobc is a process group struct member counting the number of >> processes with [...] >> [W]e caught issues that pg_jobc can go negative. [...] >> Is going negative in the first place a real bug? >I would say, yes. A "count of processes with [condition]" cannot ever >truly be negative. Unfortunately it doesn't "count processes with [condition]", that's more a coincidence on how the mechanism is used. -- -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
Re: pg_jobc going negative?
Le 09/06/2020 à 02:49, Kamil Rytarowski a écrit : pg_jobc is a process group struct member counting the number of processes with a parent capable of doing job control. Once reaching 0, the process group is orphaned. With the addition of asserts checking for pg_jobc > 0 (by maxv@), we caught issues that pg_jobc can go negative. I have landed new ATF tests that trigger this reliably with ptrace(2) (src/tests/lib/libc/sys/t_ptrace_fork_wait.h r.1.7). The problem was originally triggered with GDB. There are also other sources of these asserts due to races The ptrace(2) ATF tests are reliable triggering a negative pg_jobc, however there are racy tests doing the same as triggered by syzkaller (mentioned at least in [1]). Should we allow pg_jobc going negative? I don't think so, the code is not designed to expect negative values. (Other BSDs allow this.) They don't "allow it", they just don't have a KASSERT against it. Is going negative in the first place a real bug? There were 11 bugs reported by syzbot which showed severe memory corruption in this area. While investigating the issues, I looked at the recfounting stuff and added KASSERTs for sanity checking, and then realized they were actually firing when using the different reproducers generated by syzbot. Since I put these KASSERTs, 10 of the 11 random bugs didn't trigger, and instead it is the KASSERTs that fire earlier. Listed as duplicates: https://syzkaller.appspot.com/bug?id=50a4ddd341b90cf15a4814048ff51db04347279a You can see they are all different, but all have to do with reading the group pointer, which was either freed, overwritten, not initialized, unmapped, or contained pure garbage. This is typical of refcounting bugs where a resource disappears under your feet. Only one keeps firing once in a while (KASAN); that's understandable, because even though there is a big window where the KASSERTs can fire, the underlying race is still there and there is still the chance we miss the window and get memory corruptions. In short, (1) my understanding of it is that the code is not designed to expect negative values, and (2) since I added the KASSERTs 10/11 of the random bugs didn't trigger. Big signs the bug is indeed related to refcounting. It would be nice if someone with better understanding than me of the lwp group stuff could have a look, though. Maxime
Re: pg_jobc going negative?
> pg_jobc is a process group struct member counting the number of > processes with [...] > [W]e caught issues that pg_jobc can go negative. [...] > Is going negative in the first place a real bug? I would say, yes. A "count of processes with [condition]" cannot ever truly be negative. The only question is whether the bug is in code or in the idea that the variable always accurately reflects that (conceptual) count. (Okay, there's also the question of whether it matters in practice, but in my opinion that does not affect the question of whether it's a bug, only whether the bug matters in practice.) /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
pg_jobc going negative?
pg_jobc is a process group struct member counting the number of
processes with a parent capable of doing job control. Once reaching 0,
the process group is orphaned.
With the addition of asserts checking for pg_jobc > 0 (by maxv@), we
caught issues that pg_jobc can go negative. I have landed new ATF tests
that trigger this reliably with ptrace(2)
(src/tests/lib/libc/sys/t_ptrace_fork_wait.h r.1.7). The problem was
originally triggered with GDB.
There are also other sources of these asserts due to races
The ptrace(2) ATF tests are reliable triggering a negative pg_jobc,
however there are racy tests doing the same as triggered by syzkaller
(mentioned at least in [1]).
Should we allow pg_jobc going negative? (Other BSDs allow this.)
Is going negative in the first place a real bug?
Are the races triggered by syzkaller real bugs?
[1] http://mail-index.netbsd.org/current-users/2020/05/04/msg038511.html
On 20.04.2020 18:32, Maxime Villard wrote:
> Module Name: src
> Committed By: maxv
> Date: Mon Apr 20 16:32:03 UTC 2020
>
> Modified Files:
> src/sys/kern: kern_proc.c
>
> Log Message:
> Add three KASSERTs, to detect refcount bugs.
>
> This narrows down an unknown bug in some place near, that has manifested
> itself in various forms (use-after-frees, uninit accesses, page faults,
> segmentation faults), all pointed out by syzbot.
>
> The first KASSERT in fixjobc() fires when the bug is encountered.
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.244 -r1.245 src/sys/kern/kern_proc.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>
> Modified files:
>
> Index: src/sys/kern/kern_proc.c
> diff -u src/sys/kern/kern_proc.c:1.244 src/sys/kern/kern_proc.c:1.245
> --- src/sys/kern/kern_proc.c:1.244Sun Apr 19 20:31:59 2020
> +++ src/sys/kern/kern_proc.c Mon Apr 20 16:32:03 2020
> @@ -1,4 +1,4 @@
> -/* $NetBSD: kern_proc.c,v 1.244 2020/04/19 20:31:59 thorpej Exp $ */
> +/* $NetBSD: kern_proc.c,v 1.245 2020/04/20 16:32:03 maxv Exp $ */
>
> /*-
> * Copyright (c) 1999, 2006, 2007, 2008, 2020 The NetBSD Foundation, Inc.
> @@ -62,7 +62,7 @@
> */
>
> #include
> -__KERNEL_RCSID(0, "$NetBSD: kern_proc.c,v 1.244 2020/04/19 20:31:59 thorpej
> Exp $");
> +__KERNEL_RCSID(0, "$NetBSD: kern_proc.c,v 1.245 2020/04/20 16:32:03 maxv Exp
> $");
>
> #ifdef _KERNEL_OPT
> #include "opt_kstack.h"
> @@ -554,6 +554,7 @@ proc_sessrele(struct session *ss)
> {
>
> KASSERT(mutex_owned(proc_lock));
> + KASSERT(ss->s_count > 0);
> /*
>* We keep the pgrp with the same id as the session in order to
>* stop a process being given the same pid. Since the pgrp holds
> @@ -1181,8 +1182,11 @@ fixjobc(struct proc *p, struct pgrp *pgr
> if (entering) {
> pgrp->pg_jobc++;
> p->p_lflag &= ~PL_ORPHANPG;
> - } else if (--pgrp->pg_jobc == 0)
> - orphanpg(pgrp);
> + } else {
> + KASSERT(pgrp->pg_jobc > 0);
> + if (--pgrp->pg_jobc == 0)
> + orphanpg(pgrp);
> + }
> }
>
> /*
> @@ -1197,8 +1201,11 @@ fixjobc(struct proc *p, struct pgrp *pgr
> if (entering) {
> child->p_lflag &= ~PL_ORPHANPG;
> hispgrp->pg_jobc++;
> - } else if (--hispgrp->pg_jobc == 0)
> - orphanpg(hispgrp);
> + } else {
> + KASSERT(hispgrp->pg_jobc > 0);
> + if (--hispgrp->pg_jobc == 0)
> + orphanpg(hispgrp);
> + }
> }
> }
> }
>
signature.asc
Description: OpenPGP digital signature
