[PATCH] don't do pointless NULL checks and casts before kfree() in security/

[PATCH] don't do pointless NULL checks and casts before kfree() in security/

[Jesper Juhl]

kfree() handles NULL pointers, so checking a pointer for NULL before 
calling kfree() on it is pointless. kfree() takes a void* argument and 
changing the type of a pointer before kfree()'ing it is equally pointless.
This patch removes the pointless checks for NULL and needless mucking 
about with the pointer types before kfree() for all files in security/* 
where I could locate such code.

The following files are modified by this patch:
	security/keys/key.c
	security/keys/user_defined.c
	security/selinux/hooks.c
	security/selinux/selinuxfs.c
	security/selinux/ss/conditional.c
	security/selinux/ss/policydb.c
	security/selinux/ss/services.c

(please keep me on CC if you reply)


Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

--- linux-2.6.11-mm4-orig/security/keys/key.c	2005-03-16 15:45:42.000000000 +0100
+++ linux-2.6.11-mm4/security/keys/key.c	2005-03-20 12:40:19.000000000 +0100
@@ -114,8 +114,7 @@ struct key_user *key_user_lookup(uid_t u
  found:
 	atomic_inc(&user->usage);
 	spin_unlock(&key_user_lock);
-	if (candidate)
-		kfree(candidate);
+	kfree(candidate);
  out:
 	return user;
 
--- linux-2.6.11-mm4-orig/security/keys/user_defined.c	2005-03-16 15:45:42.000000000 +0100
+++ linux-2.6.11-mm4/security/keys/user_defined.c	2005-03-20 12:41:54.000000000 +0100
@@ -182,9 +182,7 @@ static int user_match(const struct key *
  */
 static void user_destroy(struct key *key)
 {
-	struct user_key_payload *upayload = key->payload.data;
-
-	kfree(upayload);
+	kfree(key->payload.data);
 
 } /* end user_destroy() */
 
--- linux-2.6.11-mm4-orig/security/selinux/hooks.c	2005-03-16 15:45:42.000000000 +0100
+++ linux-2.6.11-mm4/security/selinux/hooks.c	2005-03-20 12:44:43.000000000 +0100
@@ -1663,9 +1663,8 @@ static int selinux_bprm_secureexec (stru
 
 static void selinux_bprm_free_security(struct linux_binprm *bprm)
 {
-	struct bprm_security_struct *bsec = bprm->security;
+	kfree(bprm->security);
 	bprm->security = NULL;
-	kfree(bsec);
 }
 
 extern struct vfsmount *selinuxfs_mount;
--- linux-2.6.11-mm4-orig/security/selinux/selinuxfs.c	2005-03-16 15:45:42.000000000 +0100
+++ linux-2.6.11-mm4/security/selinux/selinuxfs.c	2005-03-20 12:46:11.000000000 +0100
@@ -951,8 +951,7 @@ static int sel_make_bools(void)
 	u32 sid;
 
 	/* remove any existing files */
-	if (bool_pending_values)
-		kfree(bool_pending_values);
+	kfree(bool_pending_values);
 
 	sel_remove_bools(dir);
 
@@ -997,10 +996,8 @@ static int sel_make_bools(void)
 out:
 	free_page((unsigned long)page);
 	if (names) {
-		for (i = 0; i < num; i++) {
-			if (names[i])
-				kfree(names[i]);
-		}
+		for (i = 0; i < num; i++)
+			kfree(names[i]);
 		kfree(names);
 	}
 	return ret;
--- linux-2.6.11-mm4-orig/security/selinux/ss/conditional.c	2005-03-02 08:37:49.000000000 +0100
+++ linux-2.6.11-mm4/security/selinux/ss/conditional.c	2005-03-20 12:47:16.000000000 +0100
@@ -166,16 +166,14 @@ static void cond_list_destroy(struct con
 
 void cond_policydb_destroy(struct policydb *p)
 {
-	if (p->bool_val_to_struct != NULL)
-		kfree(p->bool_val_to_struct);
+	kfree(p->bool_val_to_struct);
 	avtab_destroy(&p->te_cond_avtab);
 	cond_list_destroy(p->cond_list);
 }
 
 int cond_init_bool_indexes(struct policydb *p)
 {
-	if (p->bool_val_to_struct)
-		kfree(p->bool_val_to_struct);
+	kfree(p->bool_val_to_struct);
 	p->bool_val_to_struct = (struct cond_bool_datum**)
 		kmalloc(p->p_bools.nprim * sizeof(struct cond_bool_datum*), GFP_KERNEL);
 	if (!p->bool_val_to_struct)
@@ -185,8 +183,7 @@ int cond_init_bool_indexes(struct policy
 
 int cond_destroy_bool(void *key, void *datum, void *p)
 {
-	if (key)
-		kfree(key);
+	kfree(key);
 	kfree(datum);
 	return 0;
 }
--- linux-2.6.11-mm4-orig/security/selinux/ss/policydb.c	2005-03-16 15:45:42.000000000 +0100
+++ linux-2.6.11-mm4/security/selinux/ss/policydb.c	2005-03-20 12:59:28.000000000 +0100
@@ -590,17 +590,12 @@ void policydb_destroy(struct policydb *p
 		hashtab_destroy(p->symtab[i].table);
 	}
 
-	for (i = 0; i < SYM_NUM; i++) {
-		if (p->sym_val_to_name[i])
-			kfree(p->sym_val_to_name[i]);
-	}
+	for (i = 0; i < SYM_NUM; i++)
+		kfree(p->sym_val_to_name[i]);
 
-	if (p->class_val_to_struct)
-		kfree(p->class_val_to_struct);
-	if (p->role_val_to_struct)
-		kfree(p->role_val_to_struct);
-	if (p->user_val_to_struct)
-		kfree(p->user_val_to_struct);
+	kfree(p->class_val_to_struct);
+	kfree(p->role_val_to_struct);
+	kfree(p->user_val_to_struct);
 
 	avtab_destroy(&p->te_avtab);
 
--- linux-2.6.11-mm4-orig/security/selinux/ss/services.c	2005-03-16 15:45:42.000000000 +0100
+++ linux-2.6.11-mm4/security/selinux/ss/services.c	2005-03-20 13:01:46.000000000 +0100
@@ -1703,11 +1703,9 @@ out:
 err:
 	if (*names) {
 		for (i = 0; i < *len; i++)
-			if ((*names)[i])
-				kfree((*names)[i]);
+			kfree((*names)[i]);
 	}
-	if (*values)
-		kfree(*values);
+	kfree(*values);
 	goto out;
 }
 

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/

[Ralph Corderoy]

Hi Jesper,

> kfree() handles NULL pointers, so checking a pointer for NULL before 
> calling kfree() on it is pointless.

Not necessarily.  It helps tell the reader that the pointer may be NULL
at that point.  This has come up before.

    http://groups-beta.google.com/group/linux.kernel/browse_thread/thread/bd3d6e5a29e43c73/7b43819f874295e8?q=ralph@inputplus.co.uk+persuade+lkml#7b43819f874295e8

Cheers,


Ralph.

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/

[Jesper Juhl]

On Sun, 20 Mar 2005, Ralph Corderoy wrote:

> 
> Hi Jesper,
> 
> > kfree() handles NULL pointers, so checking a pointer for NULL before 
> > calling kfree() on it is pointless.
> 
> Not necessarily.  It helps tell the reader that the pointer may be NULL
> at that point.  This has come up before.
> 
>     http://groups-beta.google.com/group/linux.kernel/browse_thread/thread/bd3d6e5a29e43c73/7b43819f874295e8?q=ralph@inputplus.co.uk+persuade+lkml#7b43819f874295e8
> 

I agree that

	if (foo->bar) {
		kfree(foo->bar);
		foo->bar = NULL;
	}

makes it easy to see that foo->bar might be NULL, but I think the 
advantages of simply

	kfree(foo->bar);
	foo->bar = NULL;

outweigh that.

Having to remember that kfree(NULL) is valid shouldn't be hard, people 
should be used to that from userspace code calling free(), and if there 
are places where it's important to remember that the pointer might be 
NULL, then a simple comment would do, wouldn't it?

	kfree(foo->bar);	/* kfree(NULL) is valid */

the short version also have the real bennefits of generating shorter and 
faster code as well as being shorter "on-screen".


-- 
Jesper Juhl

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/

[Ralph Corderoy]

Hi Jesper,

> > Not necessarily.  It helps tell the reader that the pointer may be
> > NULL at that point.  This has come up before.
> > 
> >     http://groups-beta.google.com/group/linux.kernel/browse_thread/thread/bd3d6e5a29e43c73/7b43819f874295e8?q=ralph@inputplus.co.uk+persuade+lkml#7b43819f874295e8
> > 
> 
> I agree that
> 
> 	if (foo->bar) {
> 		kfree(foo->bar);
> 		foo->bar = NULL;
> 	}
> 
> makes it easy to see that foo->bar might be NULL, but I think the 
> advantages of simply
> 
> 	kfree(foo->bar);
> 	foo->bar = NULL;
> 
> outweigh that.
> 
> Having to remember that kfree(NULL) is valid shouldn't be hard, people 
> should be used to that from userspace code calling free(),

Agreed.

> and if there are places where it's important to remember that the
> pointer might be NULL, then a simple comment would do, wouldn't it?
> 
> 	kfree(foo->bar);	/* kfree(NULL) is valid */

I'd rather be without the same comment littering the code.

> the short version also have the real bennefits of generating shorter
> and faster code as well as being shorter "on-screen".

Faster code?  I'd have thought avoiding the function call outweighed the
overhead of checking before calling.

Cheers,


Ralph.

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/

[YOSHIFUJI Hideaki / 吉藤英明]

In article <200503201331.j2KDVhm12383@blake.inputplus.co.uk> (at Sun, 20 Mar 2005 13:31:43 +0000), Ralph Corderoy <ralph@inputplus.co.uk> says:

> > the short version also have the real bennefits of generating shorter
> > and faster code as well as being shorter "on-screen".
> 
> Faster code?  I'd have thought avoiding the function call outweighed the
> overhead of checking before calling.

Modern CPU can run nicely (expecially with register parameters).
Even if even matters, we can check inline like this:

Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

===== include/linux/slab.h 1.40 vs edited =====
--- 1.40/include/linux/slab.h	2005-03-12 05:32:31 +09:00
+++ edited/include/linux/slab.h	2005-03-20 22:47:57 +09:00
@@ -106,8 +106,17 @@
 }
 
 extern void *kcalloc(size_t, size_t, int);
-extern void kfree(const void *);
+extern void __kfree(const void *);
 extern unsigned int ksize(const void *);
+
+static inline void kfree(const void *p)
+{
+#ifndef CONFIG_CC_OPTIMIZE_FOR_SIZE
+	if (!p)
+		return;
+#endif
+	__kfree(p);
+}
 
 extern int FASTCALL(kmem_cache_reap(int));
 extern int FASTCALL(kmem_ptr_validate(kmem_cache_t *cachep, void *ptr));
===== mm/slab.c 1.156 vs edited =====
--- 1.156/mm/slab.c	2005-03-10 17:38:21 +09:00
+++ edited/mm/slab.c	2005-03-20 22:42:45 +09:00
@@ -2561,7 +2561,7 @@
 EXPORT_SYMBOL(kcalloc);
 
 /**
- * kfree - free previously allocated memory
+ * __kfree - free previously allocated memory
  * @objp: pointer returned by kmalloc.
  *
  * Don't free memory not originally allocated by kmalloc()
@@ -2572,8 +2572,10 @@
 	kmem_cache_t *c;
 	unsigned long flags;
 
+#ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
 	if (!objp)
 		return;
+#endif
 	local_irq_save(flags);
 	kfree_debugcheck(objp);
 	c = GET_PAGE_CACHE(virt_to_page(objp));
@@ -2581,7 +2583,7 @@
 	local_irq_restore(flags);
 }
 
-EXPORT_SYMBOL(kfree);
+EXPORT_SYMBOL(__kfree);
 
 #ifdef CONFIG_SMP
 /**

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/

[Jesper Juhl]

On Sun, 20 Mar 2005, Ralph Corderoy wrote:

> 
> > and if there are places where it's important to remember that the
> > pointer might be NULL, then a simple comment would do, wouldn't it?
> > 
> > 	kfree(foo->bar);	/* kfree(NULL) is valid */
> 
> I'd rather be without the same comment littering the code.
> 
Me too, but if it's between having a comment or the long version of the 
code with the if() wrapped around it, then I'll personally take the 
comment.  But the actual code should be comment enough.


> > the short version also have the real bennefits of generating shorter
> > and faster code as well as being shorter "on-screen".
> 
> Faster code?  I'd have thought avoiding the function call outweighed the
> overhead of checking before calling.
> 
I haven't actually measured it, but that would be my guess from looking at 
the actual code gcc generates.
security/selinux/ss/conditional.c::cond_policydb_destroy() for instance :

The original version with the if() in place :

void cond_policydb_destroy(struct policydb *p)
{
        if (p->bool_val_to_struct)
                kfree(p->bool_val_to_struct);
        avtab_destroy(&p->te_cond_avtab);
        cond_list_destroy(p->cond_list);
}

turns out like this :

juhl@dragon:~/download/kernel/linux-2.6.11-mm4$ objdump -d -S 
security/selinux/ss/conditional.o
[...]
void cond_policydb_destroy(struct policydb *p)
{
 220:   55                      push   %ebp
 221:   89 e5                   mov    %esp,%ebp
 223:   53                      push   %ebx
 224:   89 c3                   mov    %eax,%ebx
        if (p->bool_val_to_struct)
 226:   8b 40 78                mov    0x78(%eax),%eax
 229:   85 c0                   test   %eax,%eax
 22b:   75 13                   jne    240 <cond_policydb_destroy+0x20>
                kfree(p->bool_val_to_struct);
        avtab_destroy(&p->te_cond_avtab);
 22d:   8d 43 7c                lea    0x7c(%ebx),%eax
 230:   e8 fc ff ff ff          call   231 <cond_policydb_destroy+0x11>
        cond_list_destroy(p->cond_list);
 235:   8b 83 84 00 00 00       mov    0x84(%ebx),%eax
 23b:   5b                      pop    %ebx
 23c:   c9                      leave
 23d:   eb c1                   jmp    200 <cond_list_destroy>
 23f:   90                      nop
 240:   e8 fc ff ff ff          call   241 <cond_policydb_destroy+0x21>
 245:   8d 43 7c                lea    0x7c(%ebx),%eax
 248:   e8 fc ff ff ff          call   249 <cond_policydb_destroy+0x29>
 24d:   8b 83 84 00 00 00       mov    0x84(%ebx),%eax
 253:   5b                      pop    %ebx
 254:   c9                      leave
 255:   eb a9                   jmp    200 <cond_list_destroy>
 257:   89 f6                   mov    %esi,%esi
 259:   8d bc 27 00 00 00 00    lea    0x0(%edi),%edi

[...]


Whereas the version without the if() turns out like this :

juhl@dragon:~/download/kernel/linux-2.6.11-mm4$ objdump -d -S 
security/selinux/ss/conditional.o
[...]
void cond_policydb_destroy(struct policydb *p)
{
 220:   55                      push   %ebp
 221:   89 e5                   mov    %esp,%ebp
 223:   53                      push   %ebx
 224:   89 c3                   mov    %eax,%ebx
        kfree(p->bool_val_to_struct);
 226:   8b 40 78                mov    0x78(%eax),%eax
 229:   e8 fc ff ff ff          call   22a <cond_policydb_destroy+0xa>
        avtab_destroy(&p->te_cond_avtab);
 22e:   8d 43 7c                lea    0x7c(%ebx),%eax
 231:   e8 fc ff ff ff          call   232 <cond_policydb_destroy+0x12>
        cond_list_destroy(p->cond_list);
 236:   8b 83 84 00 00 00       mov    0x84(%ebx),%eax
 23c:   5b                      pop    %ebx
 23d:   c9                      leave
 23e:   eb c0                   jmp    200 <cond_list_destroy>

[...]


First of all that's significantly shorter, so we'll gain a bit of memory 
and I'd guess it would improve cache behaviour as well (but I don't know 
enough to say for sure).
I'm also assuming that in the vast majority of cases (not just here, 
but all over the kernel) the pointer being tested will end up being !=NULL 
so we'll end up doing the function call in any case, so saving the 
conditional should be an overall win.


-- 
Jesper 

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/

[Ralph Corderoy]

Hi Jesper,

> > > the short version also have the real bennefits of generating
> > > shorter and faster code as well as being shorter "on-screen".
> > 
> > Faster code?  I'd have thought avoiding the function call outweighed
> > the overhead of checking before calling.
> 
> I haven't actually measured it, but that would be my guess from
> looking at the actual code gcc generates.
> ...
> void cond_policydb_destroy(struct policydb *p)
> {
>  220:   55                      push   %ebp
>  221:   89 e5                   mov    %esp,%ebp
>  223:   53                      push   %ebx
>  224:   89 c3                   mov    %eax,%ebx
>         if (p->bool_val_to_struct)
>  226:   8b 40 78                mov    0x78(%eax),%eax
>  229:   85 c0                   test   %eax,%eax
>  22b:   75 13                   jne    240 <cond_policydb_destroy+0x20>
>                 kfree(p->bool_val_to_struct);
>         avtab_destroy(&p->te_cond_avtab);
>  22d:   8d 43 7c                lea    0x7c(%ebx),%eax
>  230:   e8 fc ff ff ff          call   231 <cond_policydb_destroy+0x11>
>         cond_list_destroy(p->cond_list);
>  235:   8b 83 84 00 00 00       mov    0x84(%ebx),%eax
>  23b:   5b                      pop    %ebx
>  23c:   c9                      leave
>  23d:   eb c1                   jmp    200 <cond_list_destroy>
>  23f:   90                      nop
>  240:   e8 fc ff ff ff          call   241 <cond_policydb_destroy+0x21>
>  245:   8d 43 7c                lea    0x7c(%ebx),%eax
>  248:   e8 fc ff ff ff          call   249 <cond_policydb_destroy+0x29>
>  24d:   8b 83 84 00 00 00       mov    0x84(%ebx),%eax
>  253:   5b                      pop    %ebx
>  254:   c9                      leave
>  255:   eb a9                   jmp    200 <cond_list_destroy>
>  257:   89 f6                   mov    %esi,%esi
>  259:   8d bc 27 00 00 00 00    lea    0x0(%edi),%edi
> 
> [...]
> ...
> First of all that's significantly shorter, so we'll gain a bit of
> memory and I'd guess it would improve cache behaviour as well (but I
> don't know enough to say for sure).

Yes, the original's awful isn't it.  I'm used to ARM rather than x86 and
so didn't expect such bloat by having the condition.

> I'm also assuming that in the vast majority of cases (not just here,
> but all over the kernel) the pointer being tested will end up being
> !=NULL so we'll end up doing the function call in any case, so saving
> the conditional should be an overall win.

Fair enough, you've persuaded me.  Thanks for taking the time.

Cheers,


Ralph.

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/

[Stephen Smalley]

On Sun, 2005-03-20 at 13:29 +0100, Jesper Juhl wrote:
> kfree() handles NULL pointers, so checking a pointer for NULL before 
> calling kfree() on it is pointless. kfree() takes a void* argument and 
> changing the type of a pointer before kfree()'ing it is equally pointless.
> This patch removes the pointless checks for NULL and needless mucking 
> about with the pointer types before kfree() for all files in security/* 
> where I could locate such code.
> 
> The following files are modified by this patch:
> 	security/keys/key.c
> 	security/keys/user_defined.c
> 	security/selinux/hooks.c
> 	security/selinux/selinuxfs.c
> 	security/selinux/ss/conditional.c
> 	security/selinux/ss/policydb.c
> 	security/selinux/ss/services.c
> 
> (please keep me on CC if you reply)
> 
> 
> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

The diffs to selinux look fine to me, and the resulting kernel seems to
be operating without problem.  Feel free to send along to Andrew Morton.

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/selinux/

[Jesper Juhl]

Andrew, as pr Stephen's comment below I'm sending you a diff that's a 
subset of the kfree() fixes i did for security/ earlier. The patch below 
contains only the bits from security/selinux/ that Stephen ACK'ed - 
re-diff'ed against 2.6.12-rc1-mm1.

Please consider applying.

On Tue, 22 Mar 2005, Stephen Smalley wrote:

> On Sun, 2005-03-20 at 13:29 +0100, Jesper Juhl wrote:
> > kfree() handles NULL pointers, so checking a pointer for NULL before 
> > calling kfree() on it is pointless. kfree() takes a void* argument and 
> > changing the type of a pointer before kfree()'ing it is equally pointless.
> > This patch removes the pointless checks for NULL and needless mucking 
> > about with the pointer types before kfree() for all files in security/* 
> > where I could locate such code.
> > 
> > The following files are modified by this patch:
[snip]
> > 	security/selinux/hooks.c
> > 	security/selinux/selinuxfs.c
> > 	security/selinux/ss/conditional.c
> > 	security/selinux/ss/policydb.c
> > 	security/selinux/ss/services.c
> > 
> > (please keep me on CC if you reply)
> > 
> > 
> > Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
> 
> The diffs to selinux look fine to me, and the resulting kernel seems to
> be operating without problem.  Feel free to send along to Andrew Morton.
> 
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> 


Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

diff -uprN linux-2.6.12-rc1-mm1-orig/security/selinux/hooks.c linux-2.6.12-rc1-mm1/security/selinux/hooks.c
--- linux-2.6.12-rc1-mm1-orig/security/selinux/hooks.c	2005-03-21 23:12:51.000000000 +0100
+++ linux-2.6.12-rc1-mm1/security/selinux/hooks.c	2005-03-22 21:48:29.000000000 +0100
@@ -1663,9 +1663,8 @@ static int selinux_bprm_secureexec (stru
 
 static void selinux_bprm_free_security(struct linux_binprm *bprm)
 {
-	struct bprm_security_struct *bsec = bprm->security;
+	kfree(bprm->security);
 	bprm->security = NULL;
-	kfree(bsec);
 }
 
 extern struct vfsmount *selinuxfs_mount;
diff -uprN linux-2.6.12-rc1-mm1-orig/security/selinux/selinuxfs.c linux-2.6.12-rc1-mm1/security/selinux/selinuxfs.c
--- linux-2.6.12-rc1-mm1-orig/security/selinux/selinuxfs.c	2005-03-21 23:12:51.000000000 +0100
+++ linux-2.6.12-rc1-mm1/security/selinux/selinuxfs.c	2005-03-22 21:48:29.000000000 +0100
@@ -951,8 +951,7 @@ static int sel_make_bools(void)
 	u32 sid;
 
 	/* remove any existing files */
-	if (bool_pending_values)
-		kfree(bool_pending_values);
+	kfree(bool_pending_values);
 
 	sel_remove_bools(dir);
 
@@ -997,10 +996,8 @@ static int sel_make_bools(void)
 out:
 	free_page((unsigned long)page);
 	if (names) {
-		for (i = 0; i < num; i++) {
-			if (names[i])
-				kfree(names[i]);
-		}
+		for (i = 0; i < num; i++)
+			kfree(names[i]);
 		kfree(names);
 	}
 	return ret;
diff -uprN linux-2.6.12-rc1-mm1-orig/security/selinux/ss/conditional.c linux-2.6.12-rc1-mm1/security/selinux/ss/conditional.c
--- linux-2.6.12-rc1-mm1-orig/security/selinux/ss/conditional.c	2005-03-02 08:37:49.000000000 +0100
+++ linux-2.6.12-rc1-mm1/security/selinux/ss/conditional.c	2005-03-22 21:48:29.000000000 +0100
@@ -166,16 +166,14 @@ static void cond_list_destroy(struct con
 
 void cond_policydb_destroy(struct policydb *p)
 {
-	if (p->bool_val_to_struct != NULL)
-		kfree(p->bool_val_to_struct);
+	kfree(p->bool_val_to_struct);
 	avtab_destroy(&p->te_cond_avtab);
 	cond_list_destroy(p->cond_list);
 }
 
 int cond_init_bool_indexes(struct policydb *p)
 {
-	if (p->bool_val_to_struct)
-		kfree(p->bool_val_to_struct);
+	kfree(p->bool_val_to_struct);
 	p->bool_val_to_struct = (struct cond_bool_datum**)
 		kmalloc(p->p_bools.nprim * sizeof(struct cond_bool_datum*), GFP_KERNEL);
 	if (!p->bool_val_to_struct)
@@ -185,8 +183,7 @@ int cond_init_bool_indexes(struct policy
 
 int cond_destroy_bool(void *key, void *datum, void *p)
 {
-	if (key)
-		kfree(key);
+	kfree(key);
 	kfree(datum);
 	return 0;
 }
diff -uprN linux-2.6.12-rc1-mm1-orig/security/selinux/ss/policydb.c linux-2.6.12-rc1-mm1/security/selinux/ss/policydb.c
--- linux-2.6.12-rc1-mm1-orig/security/selinux/ss/policydb.c	2005-03-21 23:12:51.000000000 +0100
+++ linux-2.6.12-rc1-mm1/security/selinux/ss/policydb.c	2005-03-22 21:48:29.000000000 +0100
@@ -590,17 +590,12 @@ void policydb_destroy(struct policydb *p
 		hashtab_destroy(p->symtab[i].table);
 	}
 
-	for (i = 0; i < SYM_NUM; i++) {
-		if (p->sym_val_to_name[i])
-			kfree(p->sym_val_to_name[i]);
-	}
+	for (i = 0; i < SYM_NUM; i++)
+		kfree(p->sym_val_to_name[i]);
 
-	if (p->class_val_to_struct)
-		kfree(p->class_val_to_struct);
-	if (p->role_val_to_struct)
-		kfree(p->role_val_to_struct);
-	if (p->user_val_to_struct)
-		kfree(p->user_val_to_struct);
+	kfree(p->class_val_to_struct);
+	kfree(p->role_val_to_struct);
+	kfree(p->user_val_to_struct);
 
 	avtab_destroy(&p->te_avtab);
 
diff -uprN linux-2.6.12-rc1-mm1-orig/security/selinux/ss/services.c linux-2.6.12-rc1-mm1/security/selinux/ss/services.c
--- linux-2.6.12-rc1-mm1-orig/security/selinux/ss/services.c	2005-03-21 23:12:51.000000000 +0100
+++ linux-2.6.12-rc1-mm1/security/selinux/ss/services.c	2005-03-22 21:48:29.000000000 +0100
@@ -1703,11 +1703,9 @@ out:
 err:
 	if (*names) {
 		for (i = 0; i < *len; i++)
-			if ((*names)[i])
-				kfree((*names)[i]);
+			kfree((*names)[i]);
 	}
-	if (*values)
-		kfree(*values);
+	kfree(*values);
 	goto out;
 }
 

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/

[David Howells]

Jesper Juhl <juhl-lkml@dif.dk> wrote:

> --- linux-2.6.11-mm4-orig/security/keys/key.c	2005-03-16 15:45:42.000000000 +0100
> +++ linux-2.6.11-mm4/security/keys/key.c	2005-03-20 12:40:19.000000000 +0100
> ...
> -	if (candidate)
> -		kfree(candidate);
> +	kfree(candidate);

Looks okay to me. It's probably less efficient though, but more space
efficient.

> --- linux-2.6.11-mm4-orig/security/keys/user_defined.c	2005-03-16 15:45:42.000000000 +0100
> +++ linux-2.6.11-mm4/security/keys/user_defined.c	2005-03-20 12:41:54.000000000 +0100
> @@ -182,9 +182,7 @@ static int user_match(const struct key *
>   */
>  static void user_destroy(struct key *key)
>  {
> -	struct user_key_payload *upayload = key->payload.data;
> -
> -	kfree(upayload);
> +	kfree(key->payload.data);

There's a patch in Andrew Morton's tree that changes this to make use of RCU,
so I'd prefer you didn't do this just yet.

David

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/

[Jesper Juhl]

On Tue, 22 Mar 2005, David Howells wrote:

> 
> Jesper Juhl <juhl-lkml@dif.dk> wrote:
> 
> > --- linux-2.6.11-mm4-orig/security/keys/key.c	2005-03-16 15:45:42.000000000 +0100
> > +++ linux-2.6.11-mm4/security/keys/key.c	2005-03-20 12:40:19.000000000 +0100
> > ...
> > -	if (candidate)
> > -		kfree(candidate);
> > +	kfree(candidate);
> 
> Looks okay to me. It's probably less efficient though, but more space
> efficient.
> 
>From looking at the code gcc generates it looks to me like the bennefits
of the smaller code should outweigh the overhead of a few more function
calls - especially if the branch is usually taken (which I must admit I 
can't really tell if it will be). If the branch is rarely taken you are 
probably right.


> > --- linux-2.6.11-mm4-orig/security/keys/user_defined.c	2005-03-16 15:45:42.000000000 +0100
> > +++ linux-2.6.11-mm4/security/keys/user_defined.c	2005-03-20 12:41:54.000000000 +0100
> > @@ -182,9 +182,7 @@ static int user_match(const struct key *
> >   */
> >  static void user_destroy(struct key *key)
> >  {
> > -	struct user_key_payload *upayload = key->payload.data;
> > -
> > -	kfree(upayload);
> > +	kfree(key->payload.data);
> 
> There's a patch in Andrew Morton's tree that changes this to make use of RCU,
> so I'd prefer you didn't do this just yet.
> 
Huh? I just checked 2.6.12-rc1-mm1, and the user_destroy function still 
looks as above...  But no problem, I'll just send Andrew the bits in 
security/selinux/ for now and wait a bit with the rest. 

Thank you for your comments.


-- 
Jesper 

Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/

[Bodo Eggert]

Ralph Corderoy <ralph@inputplus.co.uk> wrote:
> Hi Jesper,

>> kfree() handles NULL pointers, so checking a pointer for NULL before
>> calling kfree() on it is pointless.
> 
> Not necessarily.  It helps tell the reader that the pointer may be NULL
> at that point.  This has come up before.

If you want to comment code, use comments.