Created attachment 207432 [details] remove NULL check and handling of it for M_WAITOK cases Attached is a patch that removes the check of malloc(9)'s return value against NULL when M_WAITOK flag is used. This does not cover all cases, but a number of them that I found with an LLVM pass and partial compilation of kernel. That pass also checks mallocarray(), realloc(), and reallocf(), but in the areas analyzed did not see the extra code. I am not sure I like the idea of removing NULL checks, but it is already handled. Are there any cases to be aware of for why the check would be necessary with M_WAITOK? If no, I will try to go through and look for others. The patch is for 12.0-RELEASE, but can adjust that as well.
It is definitely ok for malloc, mallocarray, and realloc. (We have a kernel reallocf? Huh.) Honestly, reallocf with M_WAITOK seems like a mistake — those could instead/additionally be replaced with just realloc(M_WAITOK). > Are there any cases to be aware of for why the check would be necessary with M_WAITOK? Yes, contigmalloc() can fail with M_WAITOK; some of the busdma allocation stuff can fail with M_WAITOK. Normal malloc / UMA allocations with M_WAITOK can't return NULL (just sleep indefinitely).
By the way, have you considered writing this as a coccinelle script? It seems like something that could be done with that tool.
(In reply to Conrad Meyer from comment #2) No I have not used that tool before, but it looks handy. Is there any history of it being used in any FreeBSD processes before? Thanks for the pointer!
Yes, I've used it, pfg@ has used it some, and I believe mjg@ has as well (https://reviews.freebsd.org/D21427).
Comment for posterity: the pass used to find the ones in patch: https://github.com/roachspray/mwaitok-unneeded-rv-check
Brief update: I have worked up a Coccinelle semantic patch for this, finally. I am going to do local testing and then push it here.
Created attachment 208142 [details] Coccinelle patch that generates a unified diff for this Still need to look through / basic test the generated patch, but wanted to share the Coccinelle spatch. It could probably be simplified, but seems to have found a number of cases. It took a long time (at least 12 hours) to run on the src/sys tree.
There's some way of declaring that a coccinelle variable ('buf') hasn't been reassigned since the earlier malloc call, although I don't recall how to do it. E.g., in this code, buf = malloc(M_WAITOK); ... buf = malloc(M_NOWAIT); if (buf == NULL) { /* this case shouldn't be removed */ } The null check shouldn't be removed. It's a weird and probably bad pattern to reuse the same C variable for allocation with a different policy in the same function, but possible. > Still need to look through / basic test the generated patch, but wanted to share the Coccinelle spatch. Thanks! > It took a long time (at least 12 hours) to run on the src/sys tree. Yeah, it's not fast.
(In reply to Conrad Meyer from comment #8) Ahh, really good point; thank you for bringing that up. I will look into this.
Also, I think the patch could be simplified quite a bit if we can nest the conditional matching (i.e., result cast to type or not; position of M_WAITOK in flags; even allocation function (not sure the number of arguments matter, flags is always the last one). Do the conditional expressions ("( | )" pattern) not nest?
(In reply to Conrad Meyer from comment #10) Thanks for the comments. I am attaching a new version (titled "cocci 2" in the attachments list) that simplifies some things. It probably could be a lot better, but I am new to Coccinelle. I think this may be my last round of futzing with the Coccinelle part and will start to look at the changes it results in after it runs locally. Just wanted to get a newer version up to share.
Created attachment 208157 [details] Cocci 2
(In reply to Andrew Reiter from comment #12) And this version runs in a reasonable amount of time :-P
(In reply to Andrew Reiter from comment #11) > Thanks for the comments. I am attaching a new version (titled "cocci 2" in the > attachments list) that simplifies some things. Thanks! > It probably could be a lot better, but I am new to Coccinelle. No worries, it's new to me as well. :-) > And this version runs in a reasonable amount of time :-P Even better :-).
Created attachment 208169 [details] updated patch with better coverage Ok, so the cocci 2 script seems to generate some FP cases that I had to change and this is expressed in this attachment. There may be some other cases not found ... seems braces would play a factor (i.e., match on 'if () statement'- like cases. I scanned the patch and seemed to see the cases that were being handled. The cases that seemed questionable, I looked and adjusted as needed. Tested with LINT and GENERIC... not much testing though.
(In reply to Andrew Reiter from comment #15) IOW, I think the last diff attached may be worth going forward with.
Created attachment 208185 [details] Final (?) source diff with a couple cases added that were missed by coccinelle Final (?) source diff with a couple cases added that were missed by coccinelle that I hand fixed.
A commit references this bug: Author: markj Date: Mon Jul 20 14:28:27 UTC 2020 New revision: 363367 URL: https://svnweb.freebsd.org/changeset/base/363367 Log: ext2fs: Stop checking for failures from malloc(M_WAITOK). PR: 240545 Submitted by: Andrew Reiter <arr@watson.org> Reviewed by: fsu MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D25707 Changes: head/sys/fs/ext2fs/ext2_acl.c head/sys/fs/ext2fs/ext2_alloc.c head/sys/fs/ext2fs/ext2_extents.c head/sys/fs/ext2fs/ext2_lookup.c head/sys/fs/ext2fs/ext2_vnops.c
A commit references this bug: Author: markj Date: Mon Jul 20 17:44:14 UTC 2020 New revision: 363374 URL: https://svnweb.freebsd.org/changeset/base/363374 Log: crypto(9): Stop checking for failures from malloc(M_WAITOK). PR: 240545 Submitted by: Andrew Reiter <arr@watson.org> Reviewed by: cem, delphij, jhb MFC after: 1 week Event: July 2020 Bugathon Changes: head/sys/opencrypto/cryptodev.c
A commit references this bug: Author: markj Date: Wed Jul 22 14:32:49 UTC 2020 New revision: 363420 URL: https://svnweb.freebsd.org/changeset/base/363420 Log: usb(4): Stop checking for failures from malloc(M_WAITOK). Handle the fact that parts of usb(4) can be compiled into the boot loader, where M_WAITOK does not guarantee a successful allocation. PR: 240545 Submitted by: Andrew Reiter <arr@watson.org> (original version) Reviewed by: hselasky MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D25706 Changes: head/sys/compat/linuxkpi/common/src/linux_usb.c head/sys/dev/sound/usb/uaudio.c head/sys/dev/usb/input/uhid.c head/sys/dev/usb/input/uhid_snes.c head/sys/dev/usb/storage/ustorage_fs.c head/sys/dev/usb/usb_dev.c head/sys/dev/usb/usb_device.c head/sys/dev/usb/usb_freebsd.h head/sys/dev/usb/usb_freebsd_loader.h head/sys/dev/usb/usb_generic.c head/sys/dev/usb/usb_mbuf.c head/sys/dev/usb/usb_transfer.c
A commit references this bug: Author: markj Date: Thu Jul 23 14:03:25 UTC 2020 New revision: 363445 URL: https://svnweb.freebsd.org/changeset/base/363445 Log: ntb: Stop checking for failures from malloc(M_WAITOK). PR: 240545 Submitted by: Andrew Reiter <arr@watson.org> Reviewed by: cem, mav MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D25768 Changes: head/sys/dev/ntb/test/ntb_tool.c
A commit references this bug: Author: markj Date: Thu Jul 23 14:03:38 UTC 2020 New revision: 363446 URL: https://svnweb.freebsd.org/changeset/base/363446 Log: cuse: Stop checking for failures from malloc(M_WAITOK). PR: 240545 Submitted by: Andrew Reiter <arr@watson.org> Reviewed by: hselasky MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D25765 Changes: head/sys/fs/cuse/cuse.c
A commit references this bug: Author: markj Date: Mon Jul 27 14:14:07 UTC 2020 New revision: 363604 URL: https://svnweb.freebsd.org/changeset/base/363604 Log: MFC r363367: ext2fs: Stop checking for failures from malloc(M_WAITOK). PR: 240545 Changes: _U stable/12/ stable/12/sys/fs/ext2fs/ext2_acl.c stable/12/sys/fs/ext2fs/ext2_alloc.c stable/12/sys/fs/ext2fs/ext2_extents.c stable/12/sys/fs/ext2fs/ext2_lookup.c stable/12/sys/fs/ext2fs/ext2_vnops.c
A commit references this bug: Author: markj Date: Mon Jul 27 14:16:28 UTC 2020 New revision: 363606 URL: https://svnweb.freebsd.org/changeset/base/363606 Log: MFC r363374: crypto(9): Stop checking for failures from malloc(M_WAITOK). PR: 240545 Changes: _U stable/12/ stable/12/sys/opencrypto/cryptodev.c
A commit references this bug: Author: markj Date: Mon Jul 27 14:28:56 UTC 2020 New revision: 363608 URL: https://svnweb.freebsd.org/changeset/base/363608 Log: mpr(4), mps(4): Stop checking for failures from malloc(M_WAITOK). PR: 240545 Submitted by: Andrew Reiter <arr@watson.org> Reviewed by: imp MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D25766 Changes: head/sys/dev/mpr/mpr.c head/sys/dev/mpr/mpr_sas.c head/sys/dev/mpr/mpr_user.c head/sys/dev/mps/mps.c head/sys/dev/mps/mps_sas.c head/sys/dev/mps/mps_user.c
A commit references this bug: Author: markj Date: Mon Jul 27 19:05:54 UTC 2020 New revision: 363623 URL: https://svnweb.freebsd.org/changeset/base/363623 Log: cxgbe(4): Stop checking for failures from malloc(M_WAITOK). PR: 240545 Submitted by: Andrew Reiter <arr@watson.org> Reviewed by: np MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D25767 Changes: head/sys/dev/cxgbe/t4_main.c
while I believe there are other remaining cases to be adjusted, I think a majority have be changed. I guess I am writing this to ask if it is worth marking as closed? I appreciate the assistance in getting some changes in.
(In reply to Andrew Reiter from comment #27) Since this is about a mostly-cosmetic issue, I don't feel too strongly. If you have the time to write more patches to remove these checks, I'd be willing to apply them. I just ask that you 1) try to limit patches to a single subsystem/driver, 2) attach "git format-patch" output so that it's easy for me to apply and push patches. If you prefer not to spend more time on this, let's close the PR. Thanks for the patches thus far.