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 <[email protected]>
--- 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;
}
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/[email protected]+persuade+lkml#7b43819f874295e8
Cheers,
Ralph.
Ralph Corderoy <[email protected]> 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.
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/[email protected]+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
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/[email protected]+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.
In article <[email protected]> (at Sun, 20 Mar 2005 13:31:43 +0000), Ralph Corderoy <[email protected]> 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 <[email protected]>
===== 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
/**
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
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.
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 <[email protected]>
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 <[email protected]>
Jesper Juhl <[email protected]> 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
On Tue, 22 Mar 2005, David Howells wrote:
>
> Jesper Juhl <[email protected]> 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
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 <[email protected]>
>
> 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 <[email protected]>
>
Signed-off-by: Jesper Juhl <[email protected]>
Acked-by: Stephen Smalley <[email protected]>
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;
}