| Message ID | 20230508151337.79304-1-tudor.ambarus@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: remove superfluous check that pointer is not NULL | expand |
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
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.
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
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index dfc2e223bd10..6778c6eb6e30 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -2676,7 +2676,7 @@ static int ext4_xattr_move_to_block(handle_t *handle, struct inode *inode, out: kfree(b_entry_name); - if (needs_kvfree && buffer) + if (needs_kvfree) kvfree(buffer); if (is) brelse(is->iloc.bh);
If @buffer is NULL, no operation is performed for kvfree(buffer), remove superfluous check. Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- fs/ext4/xattr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)