Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a NULL pointer dereference bug in zfs_preumount #639

Closed
wants to merge 1 commit into from

Conversation

gunnarbeutner
Copy link
Contributor

When zpl_fill_super -> zfs_domount fails (e.g. because the dataset
was destroyed before it could be successfully mounted) the subsequent
call to zpl_kill_sb -> zfs_preumount would derefence a NULL pointer.

This bug can be reproduced using this shell script:

#!/bin/sh
(
while true; do
zfs create -o mountpoint=legacz tank/bar
zfs destroy tank/bar
done
) &

(
while true; do
mount -t zfs tank/bar /mnt
umount /mnt
done
) &

When zpl_fill_super -> zfs_domount fails (e.g. because the dataset
was destroyed before it could be successfully mounted) the subsequent
call to zpl_kill_sb -> zfs_preumount would derefence a NULL pointer.

This bug can be reproduced using this shell script:

 #!/bin/sh
 (
 while true; do
 	zfs create -o mountpoint=legacz tank/bar
 	zfs destroy tank/bar
 done
 ) &

 (
 while true; do
 	mount -t zfs tank/bar /mnt
 	umount /mnt
 done
 ) &
@behlendorf behlendorf closed this in 1f0d8a5 Apr 5, 2012
@behlendorf
Copy link
Contributor

Thank you

@behlendorf behlendorf mentioned this pull request Apr 10, 2012
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request May 21, 2018
It is just plain unsafe to peek inside in-kernel
mutex structure and make assumptions about what kernel
does with those internal fields like owner.

Kernel is all too happy to stop doing the expected things
like tracing lock owner once you load a tainted module
like spl/zfs that is not GPL.

As such you will get instant assertion failures like this:

  VERIFY3(((*(volatile typeof((&((&zo->zo_lock)->m_mutex))->owner) *)&
      ((&((&zo->zo_lock)->m_mutex))->owner))) ==
     ((void *)0)) failed (ffff88030be28500 == (null))
  PANIC at zfs_onexit.c:104:zfs_onexit_destroy()
  Showing stack for process 3626
  CPU: 0 PID: 3626 Comm: mkfs.lustre Tainted: P OE ------------ 3.10.0-debug #1
  Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
  Call Trace:
  dump_stack+0x19/0x1b
  spl_dumpstack+0x44/0x50 [spl]
  spl_panic+0xbf/0xf0 [spl]
  zfs_onexit_destroy+0x17c/0x280 [zfs]
  zfsdev_release+0x48/0xd0 [zfs]

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Chunwei Chen <[email protected]>
Reviewed-by: Gvozden Neskovic <[email protected]>
Signed-off-by: Oleg Drokin <[email protected]>
Closes openzfs#639
Closes openzfs#632
pcd1193182 pushed a commit to pcd1193182/zfs that referenced this pull request Sep 26, 2023
)

During a zettacache index merge, memory usage can increase drastically,
e.g. from 8GB to 19GB, primarily due to memory used by the
`IndexMessage`s (which are passed from the `merge_task()` to the
`next_index_task()`) and `MergeMessage`s (which is passed from the
`next_index_task()` to the `checkpoint_task()`).

IndexMessages take 62MB each, and MergeMessages take 39MB each, due to
the allocation of `MERGE_PROGRESS_CHUNK` (1 million) entries in each of
their `entries`, `frees`, and `cache_updates` Vec's.  There can be up to
100 of each of these messages in the channels, for a total of 10GB.

This commit reduces the memory usage in two ways:

Reduce the number of IndexMessages from 100 to 10.  This has little
impact on merge performance, because the next_index_task() can process
messages at a relatively constant rate.

Reduce the size of the `frees` and `cache_updates` Vec's in the
`MergeMessage` to be just big enough to contain their used entries,
which is typically very few, because there are few frees/updates
compared to the number of `entries`.

This reduces the typical maximum memory used by these messages from 10GB
to <1GB, with negligible impact on merge performance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants