Bug 240545 - patch to remove unneeded M_WAITOK return value checks
Summary: patch to remove unneeded M_WAITOK return value checks
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Mark Johnston
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-09-12 21:28 UTC by Andrew Reiter
Modified: 2020-07-27 19:06 UTC (History)
4 users (show)

See Also:


Attachments
remove NULL check and handling of it for M_WAITOK cases (11.46 KB, patch)
2019-09-12 21:28 UTC, Andrew Reiter
no flags Details | Diff
Coccinelle patch that generates a unified diff for this (42.49 KB, text/plain)
2019-10-06 15:31 UTC, Andrew Reiter
no flags Details
Cocci 2 (21.78 KB, text/plain)
2019-10-07 20:40 UTC, Andrew Reiter
no flags Details
updated patch with better coverage (27.74 KB, patch)
2019-10-08 02:51 UTC, Andrew Reiter
no flags Details | Diff
Final (?) source diff with a couple cases added that were missed by coccinelle (28.31 KB, patch)
2019-10-08 22:20 UTC, Andrew Reiter
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Reiter 2019-09-12 21:28:35 UTC
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.
Comment 1 Conrad Meyer freebsd_committer 2019-09-12 21:38:09 UTC
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).
Comment 2 Conrad Meyer freebsd_committer 2019-09-12 21:39:22 UTC
By the way, have you considered writing this as a coccinelle script?  It seems like something that could be done with that tool.
Comment 3 Andrew Reiter 2019-09-12 21:49:11 UTC
(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!
Comment 4 Conrad Meyer freebsd_committer 2019-09-12 22:27:51 UTC
Yes, I've used it, pfg@ has used it some, and I believe mjg@ has as well (https://reviews.freebsd.org/D21427).
Comment 5 Andrew Reiter 2019-09-13 19:36:05 UTC
Comment for posterity: the pass used to find the ones in patch: https://github.com/roachspray/mwaitok-unneeded-rv-check
Comment 6 Andrew Reiter 2019-10-05 03:22:50 UTC
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.
Comment 7 Andrew Reiter 2019-10-06 15:31:11 UTC
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.
Comment 8 Conrad Meyer freebsd_committer 2019-10-06 18:36:44 UTC
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.
Comment 9 Andrew Reiter 2019-10-06 20:09:14 UTC
(In reply to Conrad Meyer from comment #8)
Ahh, really good point; thank you for bringing that up. I will look into this.
Comment 10 Conrad Meyer freebsd_committer 2019-10-06 20:29:32 UTC
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?
Comment 11 Andrew Reiter 2019-10-07 20:40:25 UTC
(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.
Comment 12 Andrew Reiter 2019-10-07 20:40:57 UTC
Created attachment 208157 [details]
Cocci 2
Comment 13 Andrew Reiter 2019-10-07 21:14:25 UTC
(In reply to Andrew Reiter from comment #12)
And this version runs in a reasonable amount of time :-P
Comment 14 Conrad Meyer freebsd_committer 2019-10-08 01:01:04 UTC
(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 :-).
Comment 15 Andrew Reiter 2019-10-08 02:51:22 UTC
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.
Comment 16 Andrew Reiter 2019-10-08 04:30:03 UTC
(In reply to Andrew Reiter from comment #15)
IOW, I think the last diff attached may be worth going forward with.
Comment 17 Andrew Reiter 2019-10-08 22:20:12 UTC
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.
Comment 18 commit-hook freebsd_committer 2020-07-20 14:29:29 UTC
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
Comment 19 commit-hook freebsd_committer 2020-07-20 17:45:05 UTC
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
Comment 20 commit-hook freebsd_committer 2020-07-22 14:33:49 UTC
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
Comment 21 commit-hook freebsd_committer 2020-07-23 14:04:12 UTC
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
Comment 22 commit-hook freebsd_committer 2020-07-23 14:04:14 UTC
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
Comment 23 commit-hook freebsd_committer 2020-07-27 14:14:30 UTC
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
Comment 24 commit-hook freebsd_committer 2020-07-27 14:16:32 UTC
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
Comment 25 commit-hook freebsd_committer 2020-07-27 14:29:36 UTC
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
Comment 26 commit-hook freebsd_committer 2020-07-27 19:06:32 UTC
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