Re: kfree(NULL)

[Steven Rostedt <rostedt@goodmis.org>]

On Sun, 2006-04-23 at 05:05 +1000, Nick Piggin wrote:
> Hua Zhong wrote:
> >  > There is a judgement to be made at each call site of kfree 
> > 
> >>(and similar functions) about whether the argument is rarely 
> >>NULL, or could often be NULL.  If the janitors have been 
> >>making this judgement, I apologise, but I haven't seen them 
> >>doing that.
> >>
> >>Paul.
> > 
> > 
> > Even if the caller passes NULL most of the time, the check should be removed.
> > 
> > It's just crazy talk to say "you should not check NULL before calling kfree, as long as you make sure it's not NULL most of the
> > time".
> 
> It can reduce readability of the code [unless it is used in error path
> simplification, kfree(something) usually suggests kfree-an-object].
> 
> If the caller passes NULL most of the time, it could be in need of
> redesign.
> 
> I don't actually like kfree(NULL) any time except error paths. It is
> subjective, not crazy talk.

I wrote a little hack that detects up to 1000 callers of kfree(NULL) and
outputs what it finds with sysrq-l.

http://marc.theaimsgroup.com/?l=linux-kernel&m=114564257500757&w=2

It found right away, two function in transaction.c from the jbd code,
that were freeing an object that sometimes gets allocated.  Andrew
Morton already submitted the patch in the -mm tree to fix it:

-       kfree(new_transaction);
+       if (unlikely(new_transaction))          /* It's usually NULL */
+               kfree(new_transaction);
        return ret;
 }
 
@@ -724,7 +725,8 @@ done:
        journal_cancel_revoke(handle, jh);
 
 out:
-       kfree(frozen_buffer);
+       if (unlikely(frozen_buffer))    /* It's usually NULL */
+               kfree(frozen_buffer);

Where he uses unlikely and nicely documents that it is usually NULL (of
course the "unlikely" sort of says that already ;)

I've been running this patched kernel for a couple of days on a mostly
idle machine, (I don't need it right now, so I just let it run) and it
has shown some more problem areas.  probably occurred when updatedb
kicked off.

Here's the dump:

SysRq : Show stats on kfree
Total number of NULL frees:      1589709
Total number of non NULL frees:  69448
Callers of NULL frees:
[       27]  c0154bcd - do_tune_cpucache+0x13d/0x230
[      631]  c025b9dd - class_device_add+0xcd/0x300
[       30]  c019523c - sysfs_d_iput+0x3c/0x8e
[       44]  c0193750 - sysfs_hash_and_remove+0xd0/0x110
[        1]  c01f4787 - kobject_cleanup+0x37/0x90
[        1]  c025bf73 - class_dev_release+0x23/0x90
[       14]  c021b615 - tty_write+0x105/0x220
[       20]  c025b5ff - class_device_del+0x16f/0x190
[        6]  c021cd34 - release_mem+0x174/0x2a0
[       79]  c011e804 - do_sysctl+0x94/0x250
[   352161]  c01aafc4 - start_this_handle+0x234/0x4b0
[   430089]  c01aba66 - do_get_write_access+0x2e6/0x5a0
[    16730]  c01abdf0 - journal_get_undo_access+0xd0/0x120
[   788641]  c01a3c9f - ext3_clear_inode+0x2f/0x40
[        3]  c0194a0c - sysfs_dir_close+0x6c/0x90
[      252]  c0304e1d - inet_sock_destruct+0xad/0x1f0
[        1]  c030a698 - ip_rt_ioctl+0xe8/0x130
[      968]  c02e2669 - ip_push_pending_frames+0x2d9/0x400
[        6]  c02d69b0 - netlink_release+0x1c0/0x300
[        5]  c02ba79b - sock_fasync+0x13b/0x150

start_this_handle and do_get_write_access have already been fixed, but
now it's looking like journal_get_undo_access and ext3_clear_inode are
problem children too.

-- Steve