Skip to contentSkip to conversation

kzfree()

PATCH In review Johannes Weiner 6 participants 24 messages Feb 16, 2009 – Feb 17, 2009

Subsystems (12)
Networking Memory management USB MM core +8 more subsystems
lore
Share & tools
lore

Johannes Weiner

author
#0 lore raw

cover kzfree()

This series introduces kzfree() and converts callsites which do memset() + kfree() explicitely.

The callsites may be incomplete, I used coccinelle [1] to find them.

Regarding the recent re-exporting of ksize() to modules and the discussion around it [2], this removes the single modular in-tree user of ksize() again (unless I overgrepped something).

kfree() uses ksize() internally to determine the size of the memory region to zero out. This could mean overhead as the size is actually that of the kmalloc cache the object is from, but memset() + kfree() isn't really the common fast path pattern.

I left out w1 because I think it doesn't need to zero the memory at all. I will take a deeper look into it and send a followup with either a kzfree() conversion or removal of the memset() alltogether.

Hannes

[1] http://www.emn.fr/x-info/coccinelle/

	@@
	expression M, S;
	@@

	- memset(M, 0, S);
	- kfree(M);
	+ kzfree(M);

(from the back of my head, no coccinelle installed on this box)

[2] http://lkml.org/lkml/2009/2/10/144

 arch/s390/crypto/prng.c             |    3 +--
 crypto/api.c                        |    5 +----
 drivers/md/dm-crypt.c               |    6 ++----
 drivers/s390/crypto/zcrypt_pcixcc.c |    3 +--
 drivers/usb/host/hwa-hc.c           |    3 +--
 drivers/usb/wusbcore/cbaf.c         |    3 +--
 fs/cifs/connect.c                   |    7 ++-----
 fs/cifs/misc.c                      |   12 ++++--------
 fs/ecryptfs/keystore.c              |    3 +--
 fs/ecryptfs/messaging.c             |    3 +--
 include/linux/slab.h                |    1 +
 mm/util.c                           |   19 +++++++++++++++++++
 net/atm/mpoa_caches.c               |   14 ++++----------
 13 files changed, 39 insertions(+), 43 deletions(-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#1 lore raw

PATCH 1/8 slab: introduce kzfree()

kzfree() is a wrapper for kfree() that additionally zeroes the underlying memory before releasing it to the slab allocator.

Signed-off-by Johannes Weiner <[email protected]>
Acked-by Pekka Enberg <[email protected]>
Cc Matt Mackall <[email protected]>
Cc Christoph Lameter <[email protected]>
Cc Nick Piggin <[email protected]>
---
 include/linux/slab.h |    1 +
 mm/util.c            |   19 +++++++++++++++++++
 2 files changed, 20 insertions(+)
include/linux/slab.h +1 -0
@@ -127,6 +127,7 @@ int kmem_ptr_validate(struct kmem_cache 
 void * __must_check __krealloc(const void *, size_t, gfp_t);
 void * __must_check krealloc(const void *, size_t, gfp_t);
 void kfree(const void *);
+void kzfree(const void *);
 size_t ksize(const void *);
 
 /*
mm/util.c +19 -0
@@ -129,6 +129,25 @@ void *krealloc(const void *p, size_t new
 }
 EXPORT_SYMBOL(krealloc);
 
+/**
+ * kzfree - like kfree but zero memory
+ * @p: object to free memory of
+ *
+ * The memory of the object @p points to is zeroed before freed.
+ * If @p is %NULL, kzfree() does nothing.
+ */
+void kzfree(const void *p)
+{
+	size_t ks;
+	void *mem = (void *)p;
+
+	if (unlikely(ZERO_OR_NULL_PTR(mem)))
+		return;
+	ks = ksize(mem);
+	memset(mem, 0, ks);
+	kfree(mem);
+}
+
 /*
  * strndup_user - duplicate an existing string from user space
  * @s: The string to duplicate

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#2 lore raw

Re: [patch 1/8] slab: introduce kzfree()

Quoted 37 lines · Johannes Weiner

kzfree() is a wrapper for kfree() that additionally zeroes the underlying memory before releasing it to the slab allocator.

Signed-off-by Johannes Weiner <[email protected]>
Acked-by Pekka Enberg <[email protected]>
Cc Matt Mackall <[email protected]>
Cc Christoph Lameter <[email protected]>
Cc Nick Piggin <[email protected]>
---
 include/linux/slab.h |    1 +
 mm/util.c            |   19 +++++++++++++++++++
 2 files changed, 20 insertions(+)
include/linux/slab.h +1 -0
@@ -127,6 +127,7 @@ int kmem_ptr_validate(struct kmem_cache 
 void * __must_check __krealloc(const void *, size_t, gfp_t);
 void * __must_check krealloc(const void *, size_t, gfp_t);
 void kfree(const void *);
+void kzfree(const void *);
 size_t ksize(const void *);
 
 /*
mm/util.c +8 -0
@@ -129,6 +129,25 @@ void *krealloc(const void *p, size_t new
 }
 EXPORT_SYMBOL(krealloc);
 
+/**
+ * kzfree - like kfree but zero memory
+ * @p: object to free memory of
+ *
+ * The memory of the object @p points to is zeroed before freed.
+ * If @p is %NULL, kzfree() does nothing.
+ */
+void kzfree(const void *p)

mm/util.c void *krealloc(const void *p, size_t new · line 149

Quoted 10 lines
mm/util.c +10 -0
+{
+	size_t ks;
+	void *mem = (void *)p;
+
+	if (unlikely(ZERO_OR_NULL_PTR(mem)))
+		return;
+	ks = ksize(mem);
+	memset(mem, 0, ks);
+	kfree(mem);
+}

Sorry, please fold this delta:

mm/util.c +1 -0
@@ -147,6 +147,7 @@ void kzfree(const void *p)
 	memset(mem, 0, ks);
 	kfree(mem);
 }
+EXPORT_SYMBOL(kzfree);
 
 /*
  * strndup_user - duplicate an existing string from user space

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#4 lore raw

Re: [patch 1/8] slab: introduce kzfree()

Quoted 4 lines · Christoph Lameter

Why would you want to zero an object on release? Is this for security?

Please give us some rationale for this. Do we need free on zero now for all allocators?

All the call-sites zero out before kfree() for security reasons. But yeah, we should put that in the patch description as well.

Johannes, I suppose it would make sense to resend the series to Andrew with all the updates?

Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#6 lore raw

PATCH 2/8 crypto: use kzfree()

In reply to Johannes Weiner

Use kzfree() instead of memset() + kfree().

Signed-off-by Johannes Weiner <[email protected]>
Cc Herbert Xu <[email protected]>
---
 crypto/api.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
crypto/api.c +1 -4
@@ -569,20 +569,17 @@ EXPORT_SYMBOL_GPL(crypto_alloc_tfm);
 void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm)
 {
 	struct crypto_alg *alg;
-	int size;
 
 	if (unlikely(!mem))
 		return;
 
 	alg = tfm->__crt_alg;
-	size = ksize(mem);
 
 	if (!tfm->exit && alg->cra_exit)
 		alg->cra_exit(tfm);
 	crypto_exit_ops(tfm);
 	crypto_mod_put(alg);
-	memset(mem, 0, size);
-	kfree(mem);
+	kzfree(mem);
 }
 EXPORT_SYMBOL_GPL(crypto_destroy_tfm);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#7 lore raw

PATCH 3/8 s390: use kzfree()

In reply to Johannes Weiner

Use kzfree() instead of memset() + kfree().

Signed-off-by Johannes Weiner <[email protected]>
Cc Martin Schwidefsky <[email protected]>
Cc Heiko Carstens <[email protected]>
---
 arch/s390/crypto/prng.c             |    3 +--
 drivers/s390/crypto/zcrypt_pcixcc.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)
arch/s390/crypto/prng.c +1 -2
@@ -201,8 +201,7 @@ out_free:
 static void __exit prng_exit(void)
 {
 	/* wipe me */
-	memset(p->buf, 0, prng_chunk_size);
-	kfree(p->buf);
+	kzfree(p->buf);
 	kfree(p);
 
 	misc_deregister(&prng_dev);
drivers/s390/crypto/zcrypt_pcixcc.c +1 -2
@@ -781,8 +781,7 @@ static long zcrypt_pcixcc_send_cprb(stru
 		/* Signal pending. */
 		ap_cancel_message(zdev->ap_dev, &ap_msg);
 out_free:
-	memset(ap_msg.message, 0x0, ap_msg.length);
-	kfree(ap_msg.message);
+	kzfree(ap_msg.message);
 	return rc;
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#8 lore raw

PATCH 4/8 md: use kzfree()

In reply to Johannes Weiner

Use kzfree() instead of memset() + kfree().

Signed-off-by Johannes Weiner <[email protected]>
Cc Alasdair Kergon <[email protected]>
---
 drivers/md/dm-crypt.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/md/dm-crypt.c +2 -4
@@ -1137,8 +1137,7 @@ bad_ivmode:
 	crypto_free_ablkcipher(tfm);
 bad_cipher:
 	/* Must zero key material before freeing */
-	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
-	kfree(cc);
+	kzfree(cc);
 	return -EINVAL;
 }
 

@@ -1164,8 +1163,7 @@ static void crypt_dtr(struct dm_target *
 	dm_put_device(ti, cc->dev);
 
 	/* Must zero key material before freeing */
-	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
-	kfree(cc);
+	kzfree(cc);
 }
 
 static int crypt_map(struct dm_target *ti, struct bio *bio,

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#9 lore raw

PATCH 5/8 usb: use kzfree()

In reply to Johannes Weiner

Use kzfree() instead of memset() + kfree().

Signed-off-by Johannes Weiner <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
---
 drivers/usb/host/hwa-hc.c   |    3 +--
 drivers/usb/wusbcore/cbaf.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)
drivers/usb/host/hwa-hc.c +1 -2
@@ -464,8 +464,7 @@ static int __hwahc_dev_set_key(struct wu
 			port_idx << 8 | iface_no,
 			keyd, keyd_len, 1000 /* FIXME: arbitrary */);
 
-	memset(keyd, 0, sizeof(*keyd));	/* clear keys etc. */
-	kfree(keyd);
+	kzfree(keyd);
 	return result;
 }
drivers/usb/wusbcore/cbaf.c +1 -2
@@ -638,8 +638,7 @@ static void cbaf_disconnect(struct usb_i
 	usb_put_intf(iface);
 	kfree(cbaf->buffer);
 	/* paranoia: clean up crypto keys */
-	memset(cbaf, 0, sizeof(*cbaf));
-	kfree(cbaf);
+	kzfree(cbaf);
 }
 
 static struct usb_device_id cbaf_id_table[] = {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#10 lore raw

PATCH 6/8 cifs: use kzfree()

In reply to Johannes Weiner

Use kzfree() instead of memset() + kfree().

Signed-off-by Johannes Weiner <[email protected]>
Cc Steve French <[email protected]>
---
 fs/cifs/connect.c |    7 ++-----
 fs/cifs/misc.c    |   12 ++++--------
 2 files changed, 6 insertions(+), 13 deletions(-)
fs/cifs/connect.c +2 -5
@@ -2433,11 +2433,8 @@ mount_fail_check:
 out:
 	/* zero out password before freeing */
 	if (volume_info) {
-		if (volume_info->password != NULL) {
-			memset(volume_info->password, 0,
-				strlen(volume_info->password));
-			kfree(volume_info->password);
-		}
+		if (volume_info->password != NULL)
+			kzfree(volume_info->password);
 		kfree(volume_info->UNC);
 		kfree(volume_info->prepath);
 		kfree(volume_info);
fs/cifs/misc.c +4 -8
@@ -97,10 +97,8 @@ sesInfoFree(struct cifsSesInfo *buf_to_f
 	kfree(buf_to_free->serverOS);
 	kfree(buf_to_free->serverDomain);
 	kfree(buf_to_free->serverNOS);
-	if (buf_to_free->password) {
-		memset(buf_to_free->password, 0, strlen(buf_to_free->password));
-		kfree(buf_to_free->password);
-	}
+	if (buf_to_free->password)
+		kzfree(buf_to_free->password);
 	kfree(buf_to_free->domainName);
 	kfree(buf_to_free);
 }

@@ -132,10 +130,8 @@ tconInfoFree(struct cifsTconInfo *buf_to
 	}
 	atomic_dec(&tconInfoAllocCount);
 	kfree(buf_to_free->nativeFileSystem);
-	if (buf_to_free->password) {
-		memset(buf_to_free->password, 0, strlen(buf_to_free->password));
-		kfree(buf_to_free->password);
-	}
+	if (buf_to_free->password)
+		kzfree(buf_to_free->password);
 	kfree(buf_to_free);
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#11 lore raw

Re: [patch 6/8] cifs: use kzfree()

Hi Johannes,

fs/cifs/connect.c mount_fail_check: · line 2437

Quoted 11 lines · Johannes Weiner
fs/cifs/connect.c +2 -5
@@ -2433,11 +2433,8 @@ mount_fail_check:
 out:
       /* zero out password before freeing */
       if (volume_info) {
-               if (volume_info->password != NULL) {
-                       memset(volume_info->password, 0,
-                               strlen(volume_info->password));
-                       kfree(volume_info->password);
-               }
+               if (volume_info->password != NULL)
+                       kzfree(volume_info->password);

The NULL check here is unnecessary.

Quoted 6 lines
fs/cifs/connect.c +0 -0
               kfree(volume_info->UNC);
               kfree(volume_info->prepath);
               kfree(volume_info);

fs/cifs/misc.c sesInfoFree(struct cifsSesInfo *buf_to_f · line 101

Quoted 9 lines
fs/cifs/misc.c +2 -4
@@ -97,10 +97,8 @@ sesInfoFree(struct cifsSesInfo *buf_to_f
       kfree(buf_to_free->serverOS);
       kfree(buf_to_free->serverDomain);
       kfree(buf_to_free->serverNOS);
-       if (buf_to_free->password) {
-               memset(buf_to_free->password, 0, strlen(buf_to_free->password));
-               kfree(buf_to_free->password);
-       }
+       if (buf_to_free->password)
+               kzfree(buf_to_free->password);

And here.

Quoted 19 lines
fs/cifs/misc.c +2 -5
       kfree(buf_to_free->domainName);
       kfree(buf_to_free);
 }

@@ -132,10 +130,8 @@ tconInfoFree(struct cifsTconInfo *buf_to
       }
       atomic_dec(&tconInfoAllocCount);
       kfree(buf_to_free->nativeFileSystem);
-       if (buf_to_free->password) {
-               memset(buf_to_free->password, 0, strlen(buf_to_free->password));
-               kfree(buf_to_free->password);
-       }
+       if (buf_to_free->password)
+               kzfree(buf_to_free->password);
       kfree(buf_to_free);
 }
 
 
 
--
Quoted 5 lines

To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#12 lore raw

Re: [patch 6/8] cifs: use kzfree()

Quoted 26 lines · Pekka Enberg

Hi Johannes,

Review mount_fail_check: · line 2437

Quoted 11 lines · Johannes Weiner
(quoted diff) +2 -5
@@ -2433,11 +2433,8 @@ mount_fail_check:
 out:
       /* zero out password before freeing */
       if (volume_info) {
-               if (volume_info->password != NULL) {
-                       memset(volume_info->password, 0,
-                               strlen(volume_info->password));
-                       kfree(volume_info->password);
-               }
+               if (volume_info->password != NULL)
+                       kzfree(volume_info->password);

The NULL check here is unnecessary.

Quoted 9 lines
(quoted diff) +0 -0
               kfree(volume_info->UNC);
               kfree(volume_info->prepath);
               kfree(volume_info);
fs/cifs/misc.c +0 -0
@@ -97,10 +97,8 @@ sesInfoFree(struct cifsSesInfo *buf_to_f
       kfree(buf_to_free->serverOS);
       kfree(buf_to_free->serverDomain);
       kfree(buf_to_free->serverNOS);
Quoted 8 lines

fs/cifs/misc.c sesInfoFree(struct cifsSesInfo *buf_to_f · line 101

Quoted 6 lines
fs/cifs/misc.c +2 -4
-       if (buf_to_free->password) {
-               memset(buf_to_free->password, 0, strlen(buf_to_free->password));
-               kfree(buf_to_free->password);
-       }
+       if (buf_to_free->password)
+               kzfree(buf_to_free->password);

And here.

Thanks, Pekka!

Here is the delta to fold into the above:

[ btw, do these require an extra SOB? If so:

Signed-off-by Johannes Weiner <[email protected]>

And for http://lkml.org/lkml/2009/2/16/184:

Signed-off-by Johannes Weiner <[email protected]> ]
fs/cifs/connect.c +1 -2
@@ -2433,8 +2433,7 @@ mount_fail_check:
 out:
 	/* zero out password before freeing */
 	if (volume_info) {
-		if (volume_info->password != NULL)
-			kzfree(volume_info->password);
+		kzfree(volume_info->password);
 		kfree(volume_info->UNC);
 		kfree(volume_info->prepath);
 		kfree(volume_info);
fs/cifs/misc.c +2 -4
@@ -97,8 +97,7 @@ sesInfoFree(struct cifsSesInfo *buf_to_f
 	kfree(buf_to_free->serverOS);
 	kfree(buf_to_free->serverDomain);
 	kfree(buf_to_free->serverNOS);
-	if (buf_to_free->password)
-		kzfree(buf_to_free->password);
+	kzfree(buf_to_free->password);
 	kfree(buf_to_free->domainName);
 	kfree(buf_to_free);
 }

@@ -130,8 +129,7 @@ tconInfoFree(struct cifsTconInfo *buf_to
 	}
 	atomic_dec(&tconInfoAllocCount);
 	kfree(buf_to_free->nativeFileSystem);
-	if (buf_to_free->password)
-		kzfree(buf_to_free->password);
+	kzfree(buf_to_free->password);
 	kfree(buf_to_free);
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Reviewed-by
#13 lore raw

Re: [patch 6/8] cifs: use kzfree()

Quoted 7 lines · Johannes Weiner

Here is the delta to fold into the above:

[ btw, do these require an extra SOB? If so:

Signed-off-by Johannes Weiner <[email protected]>

And for http://lkml.org/lkml/2009/2/16/184:

Signed-off-by Johannes Weiner <[email protected]> ]

Looks good to me. As I like to see my name in the LWN stats articles, consider the whole patch:

Reviewed-by Pekka Enberg <[email protected]>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Acked-by
#14 lore raw

Re: [patch 6/8] cifs: use kzfree()

In reply to Johannes Weiner

Looks fine to me:

Acked-by Steve French <[email protected]>
Quoted 83 lines · Johannes Weiner
Quoted 3 lines · Pekka Enberg

Hi Johannes,

On Mon, Feb 16, 2009 at 4:29 PM, Johannes Weiner <[email protected]>

Quoted 14 lines · wrote:

Review mount_fail_check: · line 2437

Quoted 11 lines
(quoted diff) +2 -5
@@ -2433,11 +2433,8 @@ mount_fail_check:
 out:
       /* zero out password before freeing */
       if (volume_info) {
-               if (volume_info->password != NULL) {
-                       memset(volume_info->password, 0,
-                               strlen(volume_info->password));
-                       kfree(volume_info->password);
-               }
+               if (volume_info->password != NULL)
+                       kzfree(volume_info->password);

The NULL check here is unnecessary.

Quoted 11 lines

fs/cifs/misc.c sesInfoFree(struct cifsSesInfo *buf_to_f · line 101

Quoted 11 lines
fs/cifs/connect.c +0 -0
               kfree(volume_info->UNC);
               kfree(volume_info->prepath);
               kfree(volume_info);
fs/cifs/misc.c +0 -2
@@ -97,10 +97,8 @@ sesInfoFree(struct cifsSesInfo *buf_to_f
       kfree(buf_to_free->serverOS);
       kfree(buf_to_free->serverDomain);
       kfree(buf_to_free->serverNOS);
-       if (buf_to_free->password) {
-               memset(buf_to_free->password, 0,

strlen(buf_to_free->password));

Quoted 6 lines

fs/cifs/misc.c sesInfoFree(struct cifsSesInfo *buf_to_f · line 101

Quoted 4 lines
fs/cifs/misc.c +2 -2
-               kfree(buf_to_free->password);
-       }
+       if (buf_to_free->password)
+               kzfree(buf_to_free->password);

And here.

Thanks, Pekka!

Here is the delta to fold into the above:

[ btw, do these require an extra SOB? If so:

Signed-off-by Johannes Weiner <[email protected]>

And for http://lkml.org/lkml/2009/2/16/184:

Signed-off-by Johannes Weiner <[email protected]> ]
fs/cifs/connect.c +1 -2
@@ -2433,8 +2433,7 @@ mount_fail_check:
 out:
       /* zero out password before freeing */
       if (volume_info) {
-               if (volume_info->password != NULL)
-                       kzfree(volume_info->password);
+               kzfree(volume_info->password);
                kfree(volume_info->UNC);
               kfree(volume_info->prepath);
               kfree(volume_info);
fs/cifs/misc.c +2 -4
@@ -97,8 +97,7 @@ sesInfoFree(struct cifsSesInfo *buf_to_f
        kfree(buf_to_free->serverOS);
       kfree(buf_to_free->serverDomain);
       kfree(buf_to_free->serverNOS);
-       if (buf_to_free->password)
-               kzfree(buf_to_free->password);
+       kzfree(buf_to_free->password);
        kfree(buf_to_free->domainName);
       kfree(buf_to_free);
 }

@@ -130,8 +129,7 @@ tconInfoFree(struct cifsTconInfo *buf_to
        }
       atomic_dec(&tconInfoAllocCount);
       kfree(buf_to_free->nativeFileSystem);
-       if (buf_to_free->password)
-               kzfree(buf_to_free->password);
+       kzfree(buf_to_free->password);
       kfree(buf_to_free);
 }
Signature · 3 lines
Thanks,

Steve
Acked-by
#15 lore raw

Re: [patch 6/8] cifs: use kzfree()

In reply to Johannes Weiner

Looks fine to me:

Acked-by Steve French <[email protected]>
Quoted 72 lines · Johannes Weiner
Quoted 26 lines · Pekka Enberg

Hi Johannes,

Review mount_fail_check: · line 2437

Quoted 11 lines · Johannes Weiner
(quoted diff) +2 -5
@@ -2433,11 +2433,8 @@ mount_fail_check:
 out:
       /* zero out password before freeing */
       if (volume_info) {
-               if (volume_info->password != NULL) {
-                       memset(volume_info->password, 0,
-                               strlen(volume_info->password));
-                       kfree(volume_info->password);
-               }
+               if (volume_info->password != NULL)
+                       kzfree(volume_info->password);

The NULL check here is unnecessary.

Quoted 9 lines
fs/cifs/connect.c +0 -0
               kfree(volume_info->UNC);
               kfree(volume_info->prepath);
               kfree(volume_info);
fs/cifs/misc.c +0 -0
@@ -97,10 +97,8 @@ sesInfoFree(struct cifsSesInfo *buf_to_f
       kfree(buf_to_free->serverOS);
       kfree(buf_to_free->serverDomain);
       kfree(buf_to_free->serverNOS);
Quoted 8 lines

fs/cifs/misc.c sesInfoFree(struct cifsSesInfo *buf_to_f · line 101

Quoted 6 lines
fs/cifs/misc.c +2 -4
-       if (buf_to_free->password) {
-               memset(buf_to_free->password, 0, strlen(buf_to_free->password));
-               kfree(buf_to_free->password);
-       }
+       if (buf_to_free->password)
+               kzfree(buf_to_free->password);

And here.

Thanks, Pekka!

Here is the delta to fold into the above:

[ btw, do these require an extra SOB? If so:

Signed-off-by Johannes Weiner <[email protected]>

And for http://lkml.org/lkml/2009/2/16/184:

Signed-off-by Johannes Weiner <[email protected]> ]
fs/cifs/connect.c +1 -2
@@ -2433,8 +2433,7 @@ mount_fail_check:
 out:
       /* zero out password before freeing */
       if (volume_info) {
-               if (volume_info->password != NULL)
-                       kzfree(volume_info->password);
+               kzfree(volume_info->password);
               kfree(volume_info->UNC);
               kfree(volume_info->prepath);
               kfree(volume_info);
fs/cifs/misc.c +1 -2
@@ -97,8 +97,7 @@ sesInfoFree(struct cifsSesInfo *buf_to_f
       kfree(buf_to_free->serverOS);
       kfree(buf_to_free->serverDomain);
       kfree(buf_to_free->serverNOS);
-       if (buf_to_free->password)
-               kzfree(buf_to_free->password);
+       kzfree(buf_to_free->password);
       kfree(buf_to_free->domainName);
       kfree(buf_to_free);
 }

fs/cifs/misc.c tconInfoFree(struct cifsTconInfo *buf_to · line 135

Quoted 9 lines
fs/cifs/misc.c +1 -2
@@ -130,8 +129,7 @@ tconInfoFree(struct cifsTconInfo *buf_to
       }
       atomic_dec(&tconInfoAllocCount);
       kfree(buf_to_free->nativeFileSystem);
-       if (buf_to_free->password)
-               kzfree(buf_to_free->password);
+       kzfree(buf_to_free->password);
       kfree(buf_to_free);
 }

--
Thanks,

Steve

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#16 lore raw

PATCH 7/8 ecryptfs: use kzfree()

In reply to Johannes Weiner

Use kzfree() instead of memset() + kfree().

Signed-off-by Johannes Weiner <[email protected]>
Cc Tyler Hicks <[email protected]>
---
 fs/ecryptfs/keystore.c  |    3 +--
 fs/ecryptfs/messaging.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)
fs/ecryptfs/keystore.c +1 -2
@@ -740,8 +740,7 @@ ecryptfs_write_tag_70_packet(char *dest,
 out_release_free_unlock:
 	crypto_free_hash(s->hash_desc.tfm);
 out_free_unlock:
-	memset(s->block_aligned_filename, 0, s->block_aligned_filename_size);
-	kfree(s->block_aligned_filename);
+	kzfree(s->block_aligned_filename);
 out_unlock:
 	mutex_unlock(s->tfm_mutex);
 out:
fs/ecryptfs/messaging.c +1 -2
@@ -291,8 +291,7 @@ int ecryptfs_exorcise_daemon(struct ecry
 	if (daemon->user_ns)
 		put_user_ns(daemon->user_ns);
 	mutex_unlock(&daemon->mux);
-	memset(daemon, 0, sizeof(*daemon));
-	kfree(daemon);
+	kzfree(daemon);
 out:
 	return rc;
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

maintainer
#17 lore raw

Re: [patch 7/8] ecryptfs: use kzfree()

Quoted 15 lines · On Mon, 16 Feb 2009 15:29:33 +0100 Johannes Weiner <[email protected]> wrote:
fs/ecryptfs/keystore.c +1 -2
@@ -740,8 +740,7 @@ ecryptfs_write_tag_70_packet(char *dest,
 out_release_free_unlock:
 	crypto_free_hash(s->hash_desc.tfm);
 out_free_unlock:
-	memset(s->block_aligned_filename, 0, s->block_aligned_filename_size);
-	kfree(s->block_aligned_filename);
+	kzfree(s->block_aligned_filename);
 out_unlock:
 	mutex_unlock(s->tfm_mutex);
 out:

fs/ecryptfs/messaging.c int ecryptfs_exorcise_daemon(struct ecry · line 297

Quoted 9 lines
fs/ecryptfs/messaging.c +1 -2
@@ -291,8 +291,7 @@ int ecryptfs_exorcise_daemon(struct ecry
 	if (daemon->user_ns)
 		put_user_ns(daemon->user_ns);
 	mutex_unlock(&daemon->mux);
-	memset(daemon, 0, sizeof(*daemon));
-	kfree(daemon);
+	kzfree(daemon);
 out:
 	return rc;
 }

Except for this one and the crypto one, which might have been done for security reasons.

Even though both of them forgot to add a comment explaining this, which is bad, wrong, stupid and irritating. Sigh.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#18 lore raw

Re: [patch 7/8] ecryptfs: use kzfree()

Quoted 21 lines · Andrew Morton
Quoted 19 lines · On Mon, 16 Feb 2009 15:29:33 +0100 Johannes Weiner <[email protected]> wrote:
fs/ecryptfs/keystore.c +1 -2
@@ -740,8 +740,7 @@ ecryptfs_write_tag_70_packet(char *dest,
 out_release_free_unlock:
 	crypto_free_hash(s->hash_desc.tfm);
 out_free_unlock:
-	memset(s->block_aligned_filename, 0, s->block_aligned_filename_size);
-	kfree(s->block_aligned_filename);
+	kzfree(s->block_aligned_filename);
 out_unlock:
 	mutex_unlock(s->tfm_mutex);
 out:
fs/ecryptfs/messaging.c +0 -1
@@ -291,8 +291,7 @@ int ecryptfs_exorcise_daemon(struct ecry
 	if (daemon->user_ns)
 		put_user_ns(daemon->user_ns);
 	mutex_unlock(&daemon->mux);
-	memset(daemon, 0, sizeof(*daemon));
Quoted 8 lines

fs/ecryptfs/messaging.c int ecryptfs_exorcise_daemon(struct ecry · line 297

Quoted 5 lines
fs/ecryptfs/messaging.c +1 -1
-	kfree(daemon);
+	kzfree(daemon);
 out:
 	return rc;
 }

Except for this one and the crypto one, which might have been done for security reasons.

Actually, only atm is not security related and should probably be dropped. I didn't convert w1 for the same reason.

Quoted 2 lines

Even though both of them forgot to add a comment explaining this, which is bad, wrong, stupid and irritating. Sigh.

Humm, I considered

	kfree(stuff->foo);
	kzfree(stuff->secret_password);
	kfree(stuff);

self-explaining.

Comments could still be added. Even easier, as kzfree() is easier to grep for then a memset() + kfree() sequence ;)

Hannes

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Acked-by on v1 7/8
#19 lore raw

Re: [patch 7/8] ecryptfs: use kzfree()

In reply to Johannes Weiner
Quoted 4 lines · Johannes Weiner

Use kzfree() instead of memset() + kfree().

Signed-off-by Johannes Weiner <[email protected]>
Cc Tyler Hicks <[email protected]>
Acked-by Tyler Hicks <[email protected]>

Thanks for kzfree() and the updates to eCryptfs - I will put it to use in the future.

Quoted 31 lines
---
 fs/ecryptfs/keystore.c  |    3 +--
 fs/ecryptfs/messaging.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)
fs/ecryptfs/keystore.c +1 -2
@@ -740,8 +740,7 @@ ecryptfs_write_tag_70_packet(char *dest,
 out_release_free_unlock:
 	crypto_free_hash(s->hash_desc.tfm);
 out_free_unlock:
-	memset(s->block_aligned_filename, 0, s->block_aligned_filename_size);
-	kfree(s->block_aligned_filename);
+	kzfree(s->block_aligned_filename);
 out_unlock:
 	mutex_unlock(s->tfm_mutex);
 out:
fs/ecryptfs/messaging.c +1 -2
@@ -291,8 +291,7 @@ int ecryptfs_exorcise_daemon(struct ecry
 	if (daemon->user_ns)
 		put_user_ns(daemon->user_ns);
 	mutex_unlock(&daemon->mux);
-	memset(daemon, 0, sizeof(*daemon));
-	kfree(daemon);
+	kzfree(daemon);
 out:
 	return rc;
 }
author
#20 lore raw

PATCH 8/8 atm: use kzfree()

In reply to Johannes Weiner

Use kzfree() instead of memset() + kfree().

Signed-off-by Johannes Weiner <[email protected]>
Cc Chas Williams <[email protected]>
---
 net/atm/mpoa_caches.c |   14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)
net/atm/mpoa_caches.c +4 -10
@@ -167,11 +167,8 @@ static int cache_hit(in_cache_entry *ent
 
 static void in_cache_put(in_cache_entry *entry)
 {
-	if (atomic_dec_and_test(&entry->use)) {
-		memset(entry, 0, sizeof(in_cache_entry));
-		kfree(entry);
-	}
-
+	if (atomic_dec_and_test(&entry->use))
+		kzfree(entry);
 	return;
 }
 

@@ -403,11 +400,8 @@ static eg_cache_entry *eg_cache_get_by_s
 
 static void eg_cache_put(eg_cache_entry *entry)
 {
-	if (atomic_dec_and_test(&entry->use)) {
-		memset(entry, 0, sizeof(eg_cache_entry));
-		kfree(entry);
-	}
-
+	if (atomic_dec_and_test(&entry->use))
+		kzfree(entry);
 	return;
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

maintainer
#21 lore raw
In reply to Johannes Weiner
Quoted 2 lines · On Mon, 16 Feb 2009 15:29:26 +0100 Johannes Weiner <[email protected]> wrote:

This series introduces kzfree() and converts callsites which do memset() + kfree() explicitely.

I dunno, this looks like putting lipstick on a pig.

What is the point in zeroing memory just before freeing it? afacit this is always done as a poor-man's poisoning operation.

But the slab allocators _already_ do poisoning, and they do it better. And they do it configurably, whereas those sites you've been looking at are permanently slowing the kernel down.

So I would cheerily merge and push patches titled "remove pointless memset before kfree".

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#22 lore raw

Hi Andrew,

Quoted 9 lines · Andrew Morton
Quoted 2 lines · On Mon, 16 Feb 2009 15:29:26 +0100 Johannes Weiner <[email protected]> wrote:

This series introduces kzfree() and converts callsites which do memset() + kfree() explicitely.

I dunno, this looks like putting lipstick on a pig.

What is the point in zeroing memory just before freeing it? afacit this is always done as a poor-man's poisoning operation.

I think they do it as security paranoia to make sure other callers don't accidentally see parts of crypto keys, passwords, and such. So I don't think we can just get rid of the memsets.

Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

maintainer
#23 lore raw
Quoted 8 lines · On Mon, 16 Feb 2009 21:58:14 +0200 Pekka Enberg <[email protected]> wrote:

Hi Andrew,

Quoted 5 lines · Andrew Morton
Quoted 2 lines · On Mon, 16 Feb 2009 15:29:26 +0100 Johannes Weiner <[email protected]> wrote:

This series introduces kzfree() and converts callsites which do memset() + kfree() explicitely.

Quoted 8 lines
Quoted 4 lines

I dunno, this looks like putting lipstick on a pig.

What is the point in zeroing memory just before freeing it? afacit this is always done as a poor-man's poisoning operation.

I think they do it as security paranoia to make sure other callers don't accidentally see parts of crypto keys, passwords, and such. So I don't think we can just get rid of the memsets.

Ok, you're right - I thought only a couple were doing that but it looks like all of them except for perhaps ATM are being non-stupid.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>