| Message ID | 20200129053357.27454-14-alxndr@bu.edu |
|---|---|
| State | New |
| Headers | show |
| Series | [v8,01/21] softmmu: split off vl.c:main() into main.c | expand |
On Wed, Jan 29, 2020 at 05:34:22AM +0000, Bulekov, Alexander wrote: > The qtest-based fuzzer makes use of forking to reset-state between > tests. Keep the callback enabled, so the call_rcu thread gets created > within the child process. > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu> > --- > vl.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/vl.c b/vl.c > index bb77935f04..cf8e2d3ebb 100644 > --- a/vl.c > +++ b/vl.c > @@ -3794,7 +3794,14 @@ void qemu_init(int argc, char **argv, char **envp) > set_memory_options(&ram_slots, &maxram_size, machine_class); > > os_daemonize(); > - rcu_disable_atfork(); > + > + /* > + * If QTest is enabled, keep the rcu_atfork enabled, since system processes > + * may be forked testing purposes (e.g. fork-server based fuzzing) > + */ > + if (!qtest_enabled()) { > + rcu_disable_atfork(); > + } I haven't reviewed the details of whether resources are leaked across fork but in general it makes sense that we want an RCU thread in the fork child: Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
On 30/01/20 15:42, Stefan Hajnoczi wrote: >> + >> + /* >> + * If QTest is enabled, keep the rcu_atfork enabled, since system processes >> + * may be forked testing purposes (e.g. fork-server based fuzzing) >> + */ >> + if (!qtest_enabled()) { >> + rcu_disable_atfork(); >> + } > I haven't reviewed the details of whether resources are leaked across > fork but in general it makes sense that we want an RCU thread in the > fork child: Note that there is a possible deadlock between fork and synchronize_rcu (see commit 73c6e40, "rcu: completely disable pthread_atfork callbacks as soon as possible", 2016-01-27): - the CPU thread is inside a RCU critical section and wants to take the BQL in order to do MMIO - the I/O thread, which is owning the BQL, forks and calls rcu_init_lock, which tries to take the rcu_sync_lock - the call_rcu thread has taken rcu_sync_lock in synchronize_rcu, but synchronize_rcu needs the CPU thread to end the critical section before returning. Therefore it would be best if the fork server could fork before a single CPU instruction is executed, and then rcu_disable_atfork could be moved right after the fork server is started (just like right now we do it right after os_daemonize). We probably talked about this before, but how do you ensure that the fork server is started before threads are created (apart from the RCU thread)? Paolo
On 200130 1824, Paolo Bonzini wrote: > On 30/01/20 15:42, Stefan Hajnoczi wrote: > >> + > >> + /* > >> + * If QTest is enabled, keep the rcu_atfork enabled, since system processes > >> + * may be forked testing purposes (e.g. fork-server based fuzzing) > >> + */ > >> + if (!qtest_enabled()) { > >> + rcu_disable_atfork(); > >> + } > > I haven't reviewed the details of whether resources are leaked across > > fork but in general it makes sense that we want an RCU thread in the > > fork child: > > Note that there is a possible deadlock between fork and synchronize_rcu > (see commit 73c6e40, "rcu: completely disable pthread_atfork callbacks > as soon as possible", 2016-01-27): > > - the CPU thread is inside a RCU critical section and wants to take the > BQL in order to do MMIO > > - the I/O thread, which is owning the BQL, forks and calls > rcu_init_lock, which tries to take the rcu_sync_lock > > - the call_rcu thread has taken rcu_sync_lock in synchronize_rcu, but > synchronize_rcu needs the CPU thread to end the critical section before > returning. > > Therefore it would be best if the fork server could fork before a single > CPU instruction is executed, and then rcu_disable_atfork could be moved > right after the fork server is started (just like right now we do it > right after os_daemonize). We probably talked about this before, but > how do you ensure that the fork server is started before threads are > created (apart from the RCU thread)? With QTest, is this still a concern, since there are no CPU instructions involved? Sometimes the fork-server starts after some I/O has already occured (eg mapping BARs and setting up VQs for virtio-net). I know we briefly talked about threads at some point, and it seems that iothreads may be a concern, if any are started before fork. Other than that, since there is no TCG/CPU thread, are there any other threads that could be a concern? -Alex > Paolo >
On 30/01/20 18:42, Alexander Bulekov wrote: > With QTest, is this still a concern, since there are no CPU instructions > involved? Sometimes the fork-server starts after some I/O has already > occured (eg mapping BARs and setting up VQs for virtio-net). I know we > briefly talked about threads at some point, and it seems that iothreads > may be a concern, if any are started before fork. Other than that, since > there is no TCG/CPU thread, are there any other threads that could be > a concern? > -Alex There is a CPU thread, it just does not do MMIO. However, it may still execute code via run_on_cpu. It's quite unlikely to have the deadlock, but if it were possible to force an early start of the fork server (at the point of os_daemonize() would be ideal) it would be cleaner and it would allow reverting this patch. This is not a NACK, just some extra info. Paolo
On Wed, Jan 29, 2020 at 05:34:22AM +0000, Bulekov, Alexander wrote: >The qtest-based fuzzer makes use of forking to reset-state between >tests. Keep the callback enabled, so the call_rcu thread gets created >within the child process. > >Signed-off-by: Alexander Bulekov <alxndr@bu.edu> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> >--- > vl.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > >diff --git a/vl.c b/vl.c >index bb77935f04..cf8e2d3ebb 100644 >--- a/vl.c >+++ b/vl.c >@@ -3794,7 +3794,14 @@ void qemu_init(int argc, char **argv, char **envp) > set_memory_options(&ram_slots, &maxram_size, machine_class); > > os_daemonize(); >- rcu_disable_atfork(); >+ >+ /* >+ * If QTest is enabled, keep the rcu_atfork enabled, since system processes >+ * may be forked testing purposes (e.g. fork-server based fuzzing) >+ */ >+ if (!qtest_enabled()) { >+ rcu_disable_atfork(); >+ } > > if (pid_file && !qemu_write_pidfile(pid_file, &err)) { > error_reportf_err(err, "cannot create PID file: "); >-- >2.23.0 > >
On 29/01/2020 06.34, Bulekov, Alexander wrote: > The qtest-based fuzzer makes use of forking to reset-state between > tests. Keep the callback enabled, so the call_rcu thread gets created > within the child process. > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu> > --- > vl.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/vl.c b/vl.c > index bb77935f04..cf8e2d3ebb 100644 > --- a/vl.c > +++ b/vl.c > @@ -3794,7 +3794,14 @@ void qemu_init(int argc, char **argv, char **envp) > set_memory_options(&ram_slots, &maxram_size, machine_class); > > os_daemonize(); > - rcu_disable_atfork(); > + > + /* > + * If QTest is enabled, keep the rcu_atfork enabled, since system processes > + * may be forked testing purposes (e.g. fork-server based fuzzing) > + */ > + if (!qtest_enabled()) { > + rcu_disable_atfork(); > + } > > if (pid_file && !qemu_write_pidfile(pid_file, &err)) { > error_reportf_err(err, "cannot create PID file: "); Hi Alexander, I think this patch might maybe not work as expected: The qtest_enabled() has been added before configure_accelerators() is called in main(). So qtest_enabled() should always return "false" and thus rcu_disabled_fork() is still called in any case... could you please double-check whether it works for you and I just made a mistake, or whether this is a bug indeed? Thanks, Thomas
On 200618 0934, Thomas Huth wrote: > On 29/01/2020 06.34, Bulekov, Alexander wrote: > > The qtest-based fuzzer makes use of forking to reset-state between > > tests. Keep the callback enabled, so the call_rcu thread gets created > > within the child process. > > > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu> > > --- > > vl.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/vl.c b/vl.c > > index bb77935f04..cf8e2d3ebb 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -3794,7 +3794,14 @@ void qemu_init(int argc, char **argv, char **envp) > > set_memory_options(&ram_slots, &maxram_size, machine_class); > > > > os_daemonize(); > > - rcu_disable_atfork(); > > + > > + /* > > + * If QTest is enabled, keep the rcu_atfork enabled, since system processes > > + * may be forked testing purposes (e.g. fork-server based fuzzing) > > + */ > > + if (!qtest_enabled()) { > > + rcu_disable_atfork(); > > + } > > > > if (pid_file && !qemu_write_pidfile(pid_file, &err)) { > > error_reportf_err(err, "cannot create PID file: "); > > Hi Alexander, > > I think this patch might maybe not work as expected: The qtest_enabled() > has been added before configure_accelerators() is called in main(). So > qtest_enabled() should always return "false" and thus > rcu_disabled_fork() is still called in any case... could you please > double-check whether it works for you and I just made a mistake, or > whether this is a bug indeed? Hi, This is a problem.. I think we can work around this by calling rcu_enable_atfork from the fuzzer, after qemu_init. I'll send a patch soon. Thanks -Alex > Thanks, > Thomas >
diff --git a/vl.c b/vl.c index bb77935f04..cf8e2d3ebb 100644 --- a/vl.c +++ b/vl.c @@ -3794,7 +3794,14 @@ void qemu_init(int argc, char **argv, char **envp) set_memory_options(&ram_slots, &maxram_size, machine_class); os_daemonize(); - rcu_disable_atfork(); + + /* + * If QTest is enabled, keep the rcu_atfork enabled, since system processes + * may be forked testing purposes (e.g. fork-server based fuzzing) + */ + if (!qtest_enabled()) { + rcu_disable_atfork(); + } if (pid_file && !qemu_write_pidfile(pid_file, &err)) { error_reportf_err(err, "cannot create PID file: ");
The qtest-based fuzzer makes use of forking to reset-state between tests. Keep the callback enabled, so the call_rcu thread gets created within the child process. Signed-off-by: Alexander Bulekov <alxndr@bu.edu> --- vl.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)