* Re: [PATCH] ext4: remove superfluous check that pointer is not NULL [not found] <20230508151337.79304-1-tudor.ambarus@linaro.org> @ 2023-05-08 16:14 ` Theodore Ts'o 2023-05-08 21:13 ` Matthew Wilcox 0 siblings, 1 reply; 3+ messages in thread From: Theodore Ts'o @ 2023-05-08 16:14 UTC (permalink / raw) To: Tudor Ambarus Cc: adilger.kernel, linux-ext4, linux-kernel, joneslee, linux-mm On Mon, May 08, 2023 at 03:13:37PM +0000, Tudor Ambarus wrote: > If @buffer is NULL, no operation is performed for kvfree(buffer), > remove superfluous check. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> I was looking at this just a few weeks ago, and I couldn't find any actual *documentation* that it was safe to call vfree(NIILL) or kvfree(NULL). The problem is there are a lot of architecture-specific functions, and unlike with kfree() there is no top-level "if (ptr == NULL) return;" in the top-level vfree() and kvfree(). So I thought about removing the NULL check for kvfree(), and ultimately chickened out, since I was afraid that there might be crashes for some obscure architecture or kernel CONFIG setup. I've added linux-mm@ for their comments, and for a plea that if it is safe to pass NULL to vfree, kvfree, kvfree_rcu, etc. that it actually be *documented* somewhere. - Ted ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ext4: remove superfluous check that pointer is not NULL 2023-05-08 16:14 ` [PATCH] ext4: remove superfluous check that pointer is not NULL Theodore Ts'o @ 2023-05-08 21:13 ` Matthew Wilcox 2023-05-09 18:53 ` Theodore Ts'o 0 siblings, 1 reply; 3+ messages in thread From: Matthew Wilcox @ 2023-05-08 21:13 UTC (permalink / raw) To: Theodore Ts'o Cc: Tudor Ambarus, adilger.kernel, linux-ext4, linux-kernel, joneslee, linux-mm On Mon, May 08, 2023 at 12:14:54PM -0400, Theodore Ts'o wrote: > On Mon, May 08, 2023 at 03:13:37PM +0000, Tudor Ambarus wrote: > > If @buffer is NULL, no operation is performed for kvfree(buffer), > > remove superfluous check. > > > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > > I was looking at this just a few weeks ago, and I couldn't find any > actual *documentation* that it was safe to call vfree(NIILL) or > kvfree(NULL). The problem is there are a lot of architecture-specific > functions, and unlike with kfree() there is no top-level "if (ptr == > NULL) return;" in the top-level vfree() and kvfree(). There doesn't need to be in kvfree(). is_vmalloc_addr() returns 'false' for NULL, so it calls kfree(), which as you note has an explicit check for ZERO_OR_NULL_PTR(). is_vmalloc_addr() also returns false for the ZERO pointer, fwiw. I agree that this should be explicitly documented as allowed, since it's not reasonable to expect users to dig through these functions to verify that such a change is safe. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ext4: remove superfluous check that pointer is not NULL 2023-05-08 21:13 ` Matthew Wilcox @ 2023-05-09 18:53 ` Theodore Ts'o 0 siblings, 0 replies; 3+ messages in thread From: Theodore Ts'o @ 2023-05-09 18:53 UTC (permalink / raw) To: Matthew Wilcox Cc: Tudor Ambarus, adilger.kernel, linux-ext4, linux-kernel, joneslee, linux-mm On Mon, May 08, 2023 at 10:13:27PM +0100, Matthew Wilcox wrote: > > > > I was looking at this just a few weeks ago, and I couldn't find any > > actual *documentation* that it was safe to call vfree(NIILL) or > > kvfree(NULL). The problem is there are a lot of architecture-specific > > functions, and unlike with kfree() there is no top-level "if (ptr == > > NULL) return;" in the top-level vfree() and kvfree(). > > There doesn't need to be in kvfree(). is_vmalloc_addr() returns 'false' > for NULL, so it calls kfree(), which as you note has an explicit check > for ZERO_OR_NULL_PTR(). is_vmalloc_addr() also returns false for the > ZERO pointer, fwiw. > > I agree that this should be explicitly documented as allowed, since it's > not reasonable to expect users to dig through these functions to verify > that such a change is safe. I seem to recall at one point looking at kvfree_rcu (at least the one argument variant), and I *thought* it would unconditionally allocate memory so it could be put on a linked list to be freed after an RCU grace period had elapsed. But I tried tracing through the huge numbers of cpp macros and other layers of #ifdef's and other abstractions, and in my conference-induced sleep depreviation, it caused my head to spin, and I gave up trying to trace it down so I had 100% confidence. So if someone could document *all* of the k[v]free_* variants whether it is safe/optimal to pass NULL to them, that would be great, thanks. - Ted ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-09 18:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20230508151337.79304-1-tudor.ambarus@linaro.org>
2023-05-08 16:14 ` [PATCH] ext4: remove superfluous check that pointer is not NULL Theodore Ts'o
2023-05-08 21:13 ` Matthew Wilcox
2023-05-09 18:53 ` Theodore Ts'o
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox