[patch 0/8] kzfree()

[patch 0/8] kzfree()

[Johannes Weiner]

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(-)

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

[Johannes Weiner]

[-- Attachment #1: slab-introduce-kzfree.patch --]
[-- Type: text/plain, Size: 1393 bytes --]

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

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: Matt Mackall <mpm@selenic.com>
Cc: Christoph Lameter <cl@linux-foundation.org>
Cc: Nick Piggin <npiggin@suse.de>
---
 include/linux/slab.h |    1 +
 mm/util.c            |   19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -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 *);
 
 /*
--- a/mm/util.c
+++ b/mm/util.c
@@ -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

[patch 2/8] crypto: use kzfree()

[Johannes Weiner]

[-- Attachment #1: crypto-use-kzfree.patch --]
[-- Type: text/plain, Size: 709 bytes --]

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

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/api.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

--- a/crypto/api.c
+++ b/crypto/api.c
@@ -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);
 

[patch 3/8] s390: use kzfree()

[Johannes Weiner]

[-- Attachment #1: s390-use-kzfree.patch --]
[-- Type: text/plain, Size: 948 bytes --]

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

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/crypto/prng.c             |    3 +--
 drivers/s390/crypto/zcrypt_pcixcc.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

--- a/arch/s390/crypto/prng.c
+++ b/arch/s390/crypto/prng.c
@@ -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);
--- a/drivers/s390/crypto/zcrypt_pcixcc.c
+++ b/drivers/s390/crypto/zcrypt_pcixcc.c
@@ -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;
 }
 

[patch 4/8] md: use kzfree()

[Johannes Weiner]

[-- Attachment #1: md-use-kzfree.patch --]
[-- Type: text/plain, Size: 812 bytes --]

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

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: Alasdair Kergon <dm-devel@redhat.com>
---
 drivers/md/dm-crypt.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -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,

[patch 5/8] usb: use kzfree()

[Johannes Weiner]

[-- Attachment #1: usb-use-kzfree.patch --]
[-- Type: text/plain, Size: 923 bytes --]

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

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/usb/host/hwa-hc.c   |    3 +--
 drivers/usb/wusbcore/cbaf.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

--- a/drivers/usb/host/hwa-hc.c
+++ b/drivers/usb/host/hwa-hc.c
@@ -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;
 }
 
--- a/drivers/usb/wusbcore/cbaf.c
+++ b/drivers/usb/wusbcore/cbaf.c
@@ -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[] = {

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

[Johannes Weiner]

[-- Attachment #1: cifs-use-kzfree.patch --]
[-- Type: text/plain, Size: 1559 bytes --]

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

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: Steve French <sfrench@samba.org>
---
 fs/cifs/connect.c |    7 ++-----
 fs/cifs/misc.c    |   12 ++++--------
 2 files changed, 6 insertions(+), 13 deletions(-)

--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -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);
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -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);
 }
 

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

[Johannes Weiner]

[-- Attachment #1: ecryptfs-use-kzfree.patch --]
[-- Type: text/plain, Size: 968 bytes --]

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

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
---
 fs/ecryptfs/keystore.c  |    3 +--
 fs/ecryptfs/messaging.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

--- a/fs/ecryptfs/keystore.c
+++ b/fs/ecryptfs/keystore.c
@@ -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:
--- a/fs/ecryptfs/messaging.c
+++ b/fs/ecryptfs/messaging.c
@@ -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;
 }

[patch 8/8] atm: use kzfree()

[Johannes Weiner]

[-- Attachment #1: atm-use-kzfree.patch --]
[-- Type: text/plain, Size: 895 bytes --]

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

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: Chas Williams <chas@cmf.nrl.navy.mil>
---
 net/atm/mpoa_caches.c |   14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

--- a/net/atm/mpoa_caches.c
+++ b/net/atm/mpoa_caches.c
@@ -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;
 }
 

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

[Pekka Enberg]

Hi Johannes,

On Mon, Feb 16, 2009 at 4:29 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> @@ -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.

>                kfree(volume_info->UNC);
>                kfree(volume_info->prepath);
>                kfree(volume_info);
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -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.

>        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 majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

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

[Johannes Weiner]

On Mon, Feb 16, 2009 at 03:29:27PM +0100, Johannes Weiner wrote:
> kzfree() is a wrapper for kfree() that additionally zeroes the
> underlying memory before releasing it to the slab allocator.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: Christoph Lameter <cl@linux-foundation.org>
> Cc: Nick Piggin <npiggin@suse.de>
> ---
>  include/linux/slab.h |    1 +
>  mm/util.c            |   19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -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 *);
>  
>  /*
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -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);
> +}

Sorry, please fold this delta:

--- a/mm/util.c
+++ b/mm/util.c
@@ -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

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

[Johannes Weiner]

On Mon, Feb 16, 2009 at 05:13:30PM +0200, Pekka Enberg wrote:
> Hi Johannes,
> 
> On Mon, Feb 16, 2009 at 4:29 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> > @@ -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.
> 
> >                kfree(volume_info->UNC);
> >                kfree(volume_info->prepath);
> >                kfree(volume_info);
> > --- a/fs/cifs/misc.c
> > +++ b/fs/cifs/misc.c
> > @@ -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.

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 <hannes@cmpxchg.org>

  And for http://lkml.org/lkml/2009/2/16/184:
  Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> ]

--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -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);
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -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);
 }
 

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

[Pekka Enberg]

On Mon, Feb 16, 2009 at 5:33 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> Here is the delta to fold into the above:
>
> [ btw, do these require an extra SOB?  If so:
>  Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
>
>  And for http://lkml.org/lkml/2009/2/16/184:
>  Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> ]

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

Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>

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

[Steve French]

Looks fine to me:

Acked-by: Steve French <sfrench@us.ibm.com>

On Mon, Feb 16, 2009 at 9:33 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Mon, Feb 16, 2009 at 05:13:30PM +0200, Pekka Enberg wrote:
> > Hi Johannes,
> >
> > On Mon, Feb 16, 2009 at 4:29 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > @@ -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.
> >
> > >                kfree(volume_info->UNC);
> > >                kfree(volume_info->prepath);
> > >                kfree(volume_info);
> > > --- a/fs/cifs/misc.c
> > > +++ b/fs/cifs/misc.c
> > > @@ -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.
>
> 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 <hannes@cmpxchg.org>
>
>  And for http://lkml.org/lkml/2009/2/16/184:
>  Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> ]
>
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -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);
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -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);
>  }
>



--
Thanks,

Steve

Re: [patch 0/8] kzfree()

[Pekka Enberg]

Hi Andrew,

Andrew Morton wrote:
> On Mon, 16 Feb 2009 15:29:26 +0100 Johannes Weiner <hannes@cmpxchg.org> 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

Re: [patch 0/8] kzfree()

[Andrew Morton]

On Mon, 16 Feb 2009 15:29:26 +0100 Johannes Weiner <hannes@cmpxchg.org> 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".

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

[Andrew Morton]

On Mon, 16 Feb 2009 15:29:33 +0100 Johannes Weiner <hannes@cmpxchg.org> wrote:

> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -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:
> --- a/fs/ecryptfs/messaging.c
> +++ b/fs/ecryptfs/messaging.c
> @@ -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.

Re: [patch 0/8] kzfree()

[Andrew Morton]

On Mon, 16 Feb 2009 21:58:14 +0200 Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> Hi Andrew,
> 
> Andrew Morton wrote:
> > On Mon, 16 Feb 2009 15:29:26 +0100 Johannes Weiner <hannes@cmpxchg.org> 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.

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.

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

[Johannes Weiner]

On Mon, Feb 16, 2009 at 12:02:04PM -0800, Andrew Morton wrote:
> On Mon, 16 Feb 2009 15:29:33 +0100 Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> > --- a/fs/ecryptfs/keystore.c
> > +++ b/fs/ecryptfs/keystore.c
> > @@ -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:
> > --- a/fs/ecryptfs/messaging.c
> > +++ b/fs/ecryptfs/messaging.c
> > @@ -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.

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

> 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

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

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

Johannes Weiner wrote:
> Use kzfree() instead of memset() + kfree().
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Tyler Hicks <tyhicks@linux.vnet.ibm.com>

Acked-by: Tyler Hicks <tyhicks@linux.vnet.ibm.com>

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

> ---
>  fs/ecryptfs/keystore.c  |    3 +--
>  fs/ecryptfs/messaging.c |    3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -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:
> --- a/fs/ecryptfs/messaging.c
> +++ b/fs/ecryptfs/messaging.c
> @@ -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;
>  }
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

[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?

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

[Pekka Enberg]

On Tue, 2009-02-17 at 10:08 -0500, Christoph Lameter wrote:
> 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

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

[Christoph Lameter]

On Tue, 17 Feb 2009, Pekka Enberg wrote:

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

Ah now when looking at the whole set I see the point.