On Wed, Jul 12, 2023 at 4:20 AM Kent Overstreet <kent.overstreet@xxxxxxxxx> wrote: > > On Tue, Jul 11, 2023 at 04:44:39PM -0700, Darrick J. Wong wrote: > > On Tue, Jul 11, 2023 at 05:51:42PM +0200, Mikulas Patocka wrote: > > > When I run the test 558 on bcachefs, it works like a fork-bomb and kills > > > the machine. The reason is that the "while" loop spawns "create_file" > > > subprocesses faster than they are able to complete. > > > Please take a step back and ask why is the test spawning processes at all? If all the test needs to do is "using up all inodes" a C helper would be a much better way to accomplish that. > > > This patch fixes the crash by limiting the number of subprocesses to 128. > > > > > > Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx> > > > > > > --- > > > tests/generic/558 | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > Index: xfstests-dev/tests/generic/558 > > > =================================================================== > > > --- xfstests-dev.orig/tests/generic/558 > > > +++ xfstests-dev/tests/generic/558 > > > @@ -48,6 +48,7 @@ echo "Create $((loop * file_per_dir)) fi > > > while [ $i -lt $loop ]; do > > > create_file $SCRATCH_MNT/testdir $file_per_dir $i >>$seqres.full 2>&1 & > > > let i=$i+1 > > > + if [ $((i % 128)) = 0 ]; then wait; fi > > > > Hm. $loop is (roughly) the number of free inodes divided by 1000. This > > test completes nearly instantly on XFS; how many free inodes does > > bcachefs report after _scratch_mount? > > > > XFS reports ~570k inodes, so it's "only" starting 570 processes. > > > > I think it's probably wise to clamp $loop to something sane, but let's > > get to the bottom of how the math went wrong and we got a forkbomb. > > It's because: > - bcachefs doesn't even report a maximum number of inodes (IIRC); > inodes are small and variable size (most fields are varints, typical > inode size is 50-100 bytes). > The purpose of this test is to "Stress test fs by using up all inodes", so it seems that the test may not be appropriate for bcachefs? _require_inode_limits should have detected that, but the expectation of "no inode limit" to be reported as 0 free inodes is obviously not a standard. I am not sure which fs, if any, meets this rule? And yet, it is important for fstests to be able to tell if FSTYP should run this test or not, so either we make _require_inode_limits be aware of specific FSTYP, like _fstyp_has_non_default_seek_data_hole where there was no standard way of detecting expected behavior, or we agree on a generic heuristic that is good enough for _require_inode_limits. > - and the kernel has a sysctl to limit the maximum number of open > files, and it's got a sane default; this is what's supposed to save > us from pinned inodes eating up all ram), but systemd conveniently > overwrites it to some absurd value... > > I'd prefer to see this fixed properly, rather than just "fixing" the > test; userspace being able to pin all kernel memory this easily is a > real bug. > > We could put a second hard cap on the maximum number of open files, and > base that on a percentage of total memory; a VFS inode is somewhere in > the ballpack of a kilobyte, so easy enough to calculate. And we could > make that percentage itself a sysctl, for the people who are really > crazy... Maybe it's not a bad idea, but I don't think it would have solved the reported fork bomb issue, so the test needs to be fixed anyway. Specifically regarding the memory overuse issue, the t_open_tmpfiles tests do have this problem. These were two attempts to fix the tests: dd2c7266 fstests: don't oom the box opening tmpfiles c42c8e6c fstests: don't oom the box opening tmpfiles (take 2) But for me, generic/531 still OOMs. Kent, on a personal note, I've been following the heated discussion lately on fsdevel, one specified argument was also ignited over "fixing the test" vs. "fixing the kernel bug" argument as you raised above. One of the good points raised also by yourself was to stick to technical arguments. I would simply like to point out that you use the terminology "sane" and "crazy" quite often in arguments, as you just did in this reply. Those are obviously not objective technical arguments and yes, I know that you use them as a figure of speech and I use them often myself, but from the outside, know that they may sound dismissive of any other opinion other than the "sane" opinion. Personally, I think that the suggestion to fix the kernel to enforce the resource automatically, "_rather_ than just "fixing" the test", is well, for lack of better words, crazy ;) I am all for trying to protect the kernel from DoS, but this is far from being a trivial project, so it does not make sense to ask for that from the contributor of this simple test patch. You did say "I'd prefer to see this.." so perhaps you did not mean that the fix for the test should be blocked, but again, to the outside (to me) this is how it sounds. Bottom line - this test does not even need to open a single file to create inodes (i.e. mknod()) and certainly does not need to spawn more than one process, so I think the test should be fixed. Thanks, Amir.
