build bug on pointer type

PATCH Roel Kluin 2 participants 4 messages Mar 13, 2009 – Mar 15, 2009

Subsystems (15)

Networking Network drivers DRM drivers Memory management +11 more subsystems

lore

Share & tools

lore

Roel Kluin

author

2009-03-13 16:17 UTC #0 lore raw

I was thinking about forcing a build bug if anyone attempts to kfree a struct sk_buff*, like this:

#define PTR_OR_BB_ON_TYPE(x, type) (&x[sizeof(char[0 -			\
			__builtin_types_compatible_p(typeof(x), type)])])

and:

#define kfree(x) _kfree(PTR_OR_BB_ON_TYPE((x), struct sk_buff*))

with _kfree as the function that is currently kfree.

It appears to work in my test.c.

I also compile this with (defconfig) and there were several errors, which I fixed, included in the patch below, and also increased sparse warnings, like this

In file included from /home/roel/dnld/src/kernel/git/linux-2.6/include/linux/vmstat.h:5,

                 from /home/roel/dnld/src/kernel/git/linux-2.6/include/linux/mm.h:595,
                 from /home/roel/dnld/src/kernel/git/linux-2.6/kernel/exit.c:7:

/home/roel/dnld/src/kernel/git/linux-2.6/include/linux/percpu.h: In function 'percpu_free':
/home/roel/dnld/src/kernel/git/linux-2.6/include/linux/percpu.h:98: warning: dereferencing 'void *' pointer

This is mostly due to wrappers for kfree that take a void* argument. and can be fixed after compiling with sparse by:

sed -n -r "s/^([^:]*):([^:]*):.*derefer.*$/\2{s\/^$.*[^[:alnum:]_]$kfree$s\\\\\\\\\(\/\\\\\\\\1_kfree\\\\\\\\\(\/} \1/p" ../logs/make_def_log_20090313164543 | while read l f; do sed -i -r "$l" "$f"; done

My question, is there something fundamentally wrong with this?

Not yet meant for inclusion, and only compile tested.

---
 block/genhd.c                      |    2 +-
 drivers/firmware/dmi-id.c          |    2 +-
 drivers/gpu/drm/i915/intel_modes.c |    2 +-
 include/linux/kernel.h             |    5 +++++
 include/linux/slab.h               |    5 ++++-
 kernel/exit.c                      |    2 +-
 lib/kref.c                         |    2 +-
 mm/slab.c                          |    4 ++--
 mm/slob.c                          |    4 ++--
 mm/slub.c                          |    4 ++--
 net/core/dev.c                     |    4 ++--
 security/selinux/ss/services.c     |    2 +-
 12 files changed, 23 insertions(+), 15 deletions(-)

block/genhd.c +1 -1

index a9ec910..e732530 100644
@@ -972,7 +972,7 @@ static void disk_release(struct device *dev)
 {
 	struct gendisk *disk = dev_to_disk(dev);
 
-	kfree(disk->random);
+	_kfree(disk->random);
 	disk_replace_part_tbl(disk, NULL);
 	free_part_stats(&disk->part0);
 	kfree(disk);

drivers/firmware/dmi-id.c +1 -1

index 5a76d05..e4517d7 100644
@@ -160,7 +160,7 @@ static int dmi_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
 
 static struct class dmi_class = {
 	.name = "dmi",
-	.dev_release = (void(*)(struct device *)) kfree,
+	.dev_release = (void(*)(struct device *)) _kfree,
 	.dev_uevent = dmi_dev_uevent,
 };

drivers/gpu/drm/i915/intel_modes.c +1 -1

index e42019e..0ca35e5 100644
@@ -76,7 +76,7 @@ int intel_ddc_get_modes(struct intel_output *intel_output)
 		drm_mode_connector_update_edid_property(&intel_output->base,
 							edid);
 		ret = drm_add_edid_modes(&intel_output->base, edid);
-		kfree(edid);
+		_kfree(edid);
 	}
 
 	return ret;

include/linux/kernel.h +5 -0

index 7fa3718..8809c9c 100644
@@ -526,6 +526,11 @@ struct sysinfo {
    aren't permitted). */
 #define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
 
+/* If the type of pointer x matches type, force a compilation error,
+   otherwise produce x as a result */
+#define PTR_OR_BB_ON_TYPE(x, type) (&x[sizeof(char[0 -			\
+			__builtin_types_compatible_p(typeof(x), type)])])
+
 /* Trap pasters of __FUNCTION__ at compile-time */
 #define __FUNCTION__ (__func__)

include/linux/slab.h +4 -1

index 24c5602..7a17c3e 100644
@@ -121,12 +121,15 @@ int kmem_ptr_validate(struct kmem_cache *cachep, const void *ptr);
 #define KMALLOC_MAX_SIZE	(1UL << KMALLOC_SHIFT_HIGH)
 #define KMALLOC_MAX_ORDER	(KMALLOC_SHIFT_HIGH - PAGE_SHIFT)
 
+/* This ensures that kfree is not called with a struct sk_buff* */
+#define kfree(x) _kfree(PTR_OR_BB_ON_TYPE((x), struct sk_buff*))
+
 /*
  * Common kmalloc functions provided by all allocators
  */
 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 _kfree(const void *);
 void kzfree(const void *);
 size_t ksize(const void *);

kernel/exit.c +1 -1

index efd30cc..96eb98a 100644
@@ -1103,7 +1103,7 @@ NORET_TYPE void do_exit(long code)
 	if (unlikely(!list_empty(&tsk->pi_state_list)))
 		exit_pi_state_list(tsk);
 	if (unlikely(current->pi_state_cache))
-		kfree(current->pi_state_cache);
+		_kfree(current->pi_state_cache);
 #endif
 	/*
 	 * Make sure we are holding no locks:

lib/kref.c +1 -1

index 9ecd6e8..a6364df 100644
@@ -62,7 +62,7 @@ void kref_get(struct kref *kref)
 int kref_put(struct kref *kref, void (*release)(struct kref *kref))
 {
 	WARN_ON(release == NULL);
-	WARN_ON(release == (void (*)(struct kref *))kfree);
+	WARN_ON(release == (void (*)(struct kref *))_kfree);
 
 	if (atomic_dec_and_test(&kref->refcount)) {
 		release(kref);

mm/slab.c +2 -2

index 4d00855..3acefac 100644
@@ -3711,7 +3711,7 @@ EXPORT_SYMBOL(kmem_cache_free);
  * Don't free memory not originally allocated by kmalloc()
  * or you will run into trouble.
  */
-void kfree(const void *objp)
+void _kfree(const void *objp)
 {
 	struct kmem_cache *c;
 	unsigned long flags;

@@ -3726,7 +3726,7 @@ void kfree(const void *objp)
 	__cache_free(c, (void *)objp);
 	local_irq_restore(flags);
 }
-EXPORT_SYMBOL(kfree);
+EXPORT_SYMBOL(_kfree);
 
 unsigned int kmem_cache_size(struct kmem_cache *cachep)
 {

mm/slob.c +2 -2

index 52bc8a2..1d8a3c4 100644
@@ -487,7 +487,7 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
 }
 EXPORT_SYMBOL(__kmalloc_node);
 
-void kfree(const void *block)
+void _kfree(const void *block)
 {
 	struct slob_page *sp;

@@ -502,7 +502,7 @@ void kfree(const void *block)
 	} else
 		put_page(&sp->page);
 }
-EXPORT_SYMBOL(kfree);
+EXPORT_SYMBOL(_kfree);
 
 /* can't use ksize for kmem_cache_alloc memory, only kmalloc */
 size_t ksize(const void *block)

mm/slub.c +2 -2

index 0280eee..f9fa9e1 100644
@@ -2738,7 +2738,7 @@ size_t ksize(const void *object)
 }
 EXPORT_SYMBOL(ksize);
 
-void kfree(const void *x)
+void _kfree(const void *x)
 {
 	struct page *page;
 	void *object = (void *)x;

@@ -2754,7 +2754,7 @@ void kfree(const void *x)
 	}
 	slab_free(page->slab, page, object, _RET_IP_);
 }
-EXPORT_SYMBOL(kfree);
+EXPORT_SYMBOL(_kfree);
 
 /*
  * kmem_cache_shrink removes empty slabs from the partial lists and sorts

net/core/dev.c +2 -2

index f112970..74038ef 100644
@@ -2671,7 +2671,7 @@ void netif_napi_del(struct napi_struct *napi)
 	struct sk_buff *skb, *next;
 
 	list_del_init(&napi->dev_list);
-	kfree(napi->skb);
+	_kfree(napi->skb);
 
 	for (skb = napi->gro_list; skb; skb = next) {
 		next = skb->next;

@@ -4720,7 +4720,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	if (!tx) {
 		printk(KERN_ERR "alloc_netdev: Unable to allocate "
 		       "tx qdiscs.\n");
-		kfree(p);
+		_kfree(p);
 		return NULL;
 	}

security/selinux/ss/services.c +1 -1

index c65e4fe..cefe043 100644
@@ -2862,7 +2862,7 @@ static void security_netlbl_cache_add(struct netlbl_lsm_secattr *secattr,
 	}
 
 	*sid_cache = sid;
-	secattr->cache->free = kfree;
+	secattr->cache->free = _kfree;
 	secattr->cache->data = sid_cache;
 	secattr->flags |= NETLBL_SECATTR_CACHE;
 }

Roel Kluin

author

2009-03-13 23:16 UTC #1 lore raw

build bug on kfree(struct sk_buff*)

Hi David,

Maybe I should have sent this to you (and netdev) in the first place.

Quoted 6 lines · I wrote:

I was thinking about forcing a build bug if anyone attempts to kfree a struct sk_buff*, like this:
#define PTR_OR_BB_ON_TYPE(x, type) (&x[sizeof(char[0 -			\
			__builtin_types_compatible_p(typeof(x), type)])])

Quoted 7 lines

and:
#define kfree(x) _kfree(PTR_OR_BB_ON_TYPE((x), struct sk_buff*))
with _kfree as the function that is currently kfree.
It appears to work in my test.c.

My question is, is there something wrong with this?

Quoted 2 lines

I also compiled this with (defconfig) and there were breakages, which are fixed in the patch below.

I am now trying allyesconfig. The errors are due to:

* In net/core/dev.c +2674 a struct sk_buff* was freed,

* when assigning kfree to a function pointer.

* Several kfrees, but I don't know why gcc complains:

In block/genhd.c:
struct timer_rand_state *random,

In drivers/gpu/drm/i915/intel_modes.c:
struct edid *edid,

In kernel/exit.c:
struct futex_pi_state *pi_state_cache;

In drivers/char/ipmi/ipmi_si_intf.c:
struct si_sm_data *si_sm;

e.g:
/home/roel/dnld/src/kernel/git/linux-2.6/drivers/char/ipmi/ipmi_si_intf.c: In function 'try_smi_init':
/home/roel/dnld/src/kernel/git/linux-2.6/drivers/char/ipmi/ipmi_si_intf.c:2960: error: invalid use of undefined type 'struct si_sm_data' /home/roel/dnld/src/kernel/git/linux-2.6/drivers/char/ipmi/ipmi_si_intf.c:2960: error: dereferencing pointer to incomplete type /home/roel/dnld/src/kernel/git/linux-2.6/drivers/char/ipmi/ipmi_si_intf.c: In function 'cleanup_one_si':
/home/roel/dnld/src/kernel/git/linux-2.6/drivers/char/ipmi/ipmi_si_intf.c:3123: error: invalid use of undefined type 'struct si_sm_data' /home/roel/dnld/src/kernel/git/linux-2.6/drivers/char/ipmi/ipmi_si_intf.c:3123: error: dereferencing pointer to incomplete type

However, allyes wasn't finished yet.

Quoted 9 lines

Also increased sparse warnings, like this:
In file included from /home/roel/dnld/src/kernel/git/linux-2.6/include/linux/vmstat.h:5,
                 from /home/roel/dnld/src/kernel/git/linux-2.6/include/linux/mm.h:595,
                 from /home/roel/dnld/src/kernel/git/linux-2.6/kernel/exit.c:7:
/home/roel/dnld/src/kernel/git/linux-2.6/include/linux/percpu.h: In function 'percpu_free':
/home/roel/dnld/src/kernel/git/linux-2.6/include/linux/percpu.h:98: warning: dereferencing 'void *' pointer
This is mostly due to wrappers for kfree that take a void* argument

But also in cases where the variable is a void*, e.g. net/core/dev.c +4720 below.

I can quite easily change these kfree's to _kfree, however, I am not sure if that's the best strategy.

Roel

Not meant for inclusion (yet)

---
 block/genhd.c                      |    2 +-
 drivers/char/ipmi/ipmi_si_intf.c   |    4 ++--
 drivers/firmware/dmi-id.c          |    2 +-
 drivers/gpu/drm/i915/intel_modes.c |    2 +-
 drivers/s390/char/vmlogrdr.c       |    2 +-
 drivers/s390/net/netiucv.c         |    2 +-
 fs/nfsd/nfs4xdr.c                  |    4 ++--
 include/linux/kernel.h             |    5 +++++
 include/linux/slab.h               |    5 ++++-
 kernel/exit.c                      |    2 +-
 lib/kref.c                         |    2 +-
 mm/slab.c                          |    4 ++--
 mm/slob.c                          |    4 ++--
 mm/slub.c                          |    4 ++--
 net/core/dev.c                     |    4 ++--
 security/selinux/ss/services.c     |    2 +-
 16 files changed, 29 insertions(+), 21 deletions(-)

block/genhd.c +1 -1

index a9ec910..e732530 100644
@@ -972,7 +972,7 @@ static void disk_release(struct device *dev)
 {
 	struct gendisk *disk = dev_to_disk(dev);
 
-	kfree(disk->random);
+	_kfree(disk->random);
 	disk_replace_part_tbl(disk, NULL);
 	free_part_stats(&disk->part0);
 	kfree(disk);

drivers/char/ipmi/ipmi_si_intf.c +2 -2

index 3000135..f86798d 100644
@@ -2957,7 +2957,7 @@ static int try_smi_init(struct smi_info *new_smi)
 	if (new_smi->si_sm) {
 		if (new_smi->handlers)
 			new_smi->handlers->cleanup(new_smi->si_sm);
-		kfree(new_smi->si_sm);
+		_kfree(new_smi->si_sm);
 	}
 	if (new_smi->addr_source_cleanup)
 		new_smi->addr_source_cleanup(new_smi);

@@ -3120,7 +3120,7 @@ static void cleanup_one_si(struct smi_info *to_clean)
 
 	to_clean->handlers->cleanup(to_clean->si_sm);
 
-	kfree(to_clean->si_sm);
+	_kfree(to_clean->si_sm);
 
 	if (to_clean->addr_source_cleanup)
 		to_clean->addr_source_cleanup(to_clean);

drivers/firmware/dmi-id.c +1 -1

index 5a76d05..e4517d7 100644
@@ -160,7 +160,7 @@ static int dmi_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
 
 static struct class dmi_class = {
 	.name = "dmi",
-	.dev_release = (void(*)(struct device *)) kfree,
+	.dev_release = (void(*)(struct device *)) _kfree,
 	.dev_uevent = dmi_dev_uevent,
 };

drivers/gpu/drm/i915/intel_modes.c +1 -1

index e42019e..0ca35e5 100644
@@ -76,7 +76,7 @@ int intel_ddc_get_modes(struct intel_output *intel_output)
 		drm_mode_connector_update_edid_property(&intel_output->base,
 							edid);
 		ret = drm_add_edid_modes(&intel_output->base, edid);
-		kfree(edid);
+		_kfree(edid);
 	}
 
 	return ret;

drivers/s390/char/vmlogrdr.c +1 -1

index d8a2289..35471bc 100644
@@ -736,7 +736,7 @@ static int vmlogrdr_register_device(struct vmlogrdr_priv_t *priv)
 		 * directly here. (Probably a little bit obfuscating
 		 * but legitime ...).
 		 */
-		dev->release = (void (*)(struct device *))kfree;
+		dev->release = (void (*)(struct device *))_kfree;
 	} else
 		return -ENOMEM;
 	ret = device_register(dev);

drivers/s390/net/netiucv.c +1 -1

index 930e2fc..2ab4e65 100644
@@ -1745,7 +1745,7 @@ static int netiucv_register_device(struct net_device *ndev)
 		 * directly here. (Probably a little bit obfuscating
 		 * but legitime ...).
 		 */
-		dev->release = (void (*)(struct device *))kfree;
+		dev->release = (void (*)(struct device *))_kfree;
 		dev->driver = &netiucv_driver;
 	} else
 		return -ENOMEM;

fs/nfsd/nfs4xdr.c +2 -2

index f65953b..3c09abd 100644
@@ -215,7 +215,7 @@ static char *savemem(struct nfsd4_compoundargs *argp, __be32 *p, int nbytes)
 		BUG_ON(p != argp->tmpp);
 		argp->tmpp = NULL;
 	}
-	if (defer_free(argp, kfree, p)) {
+	if (defer_free(argp, _kfree, p)) {
 		kfree(p);
 		return NULL;
 	} else

@@ -292,7 +292,7 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval, struct iattr *ia
 			host_err = -ENOMEM;
 			goto out_nfserr;
 		}
-		defer_free(argp, kfree, *acl);
+		defer_free(argp, _kfree, *acl);
 
 		(*acl)->naces = nace;
 		for (ace = (*acl)->aces; ace < (*acl)->aces + nace; ace++) {

include/linux/kernel.h +5 -0

index 7fa3718..8809c9c 100644
@@ -526,6 +526,11 @@ struct sysinfo {
    aren't permitted). */
 #define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
 
+/* If the type of pointer x matches type, force a compilation error,
+   otherwise produce x as a result */
+#define PTR_OR_BB_ON_TYPE(x, type) (&x[sizeof(char[0 -			\
+			__builtin_types_compatible_p(typeof(x), type)])])
+
 /* Trap pasters of __FUNCTION__ at compile-time */
 #define __FUNCTION__ (__func__)

include/linux/slab.h +4 -1

index 24c5602..7a17c3e 100644
@@ -121,12 +121,15 @@ int kmem_ptr_validate(struct kmem_cache *cachep, const void *ptr);
 #define KMALLOC_MAX_SIZE	(1UL << KMALLOC_SHIFT_HIGH)
 #define KMALLOC_MAX_ORDER	(KMALLOC_SHIFT_HIGH - PAGE_SHIFT)
 
+/* This ensures that kfree is not called with a struct sk_buff* */
+#define kfree(x) _kfree(PTR_OR_BB_ON_TYPE((x), struct sk_buff*))
+
 /*
  * Common kmalloc functions provided by all allocators
  */
 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 _kfree(const void *);
 void kzfree(const void *);
 size_t ksize(const void *);

kernel/exit.c +1 -1

index efd30cc..96eb98a 100644
@@ -1103,7 +1103,7 @@ NORET_TYPE void do_exit(long code)
 	if (unlikely(!list_empty(&tsk->pi_state_list)))
 		exit_pi_state_list(tsk);
 	if (unlikely(current->pi_state_cache))
-		kfree(current->pi_state_cache);
+		_kfree(current->pi_state_cache);
 #endif
 	/*
 	 * Make sure we are holding no locks:

lib/kref.c +1 -1

index 9ecd6e8..a6364df 100644
@@ -62,7 +62,7 @@ void kref_get(struct kref *kref)
 int kref_put(struct kref *kref, void (*release)(struct kref *kref))
 {
 	WARN_ON(release == NULL);
-	WARN_ON(release == (void (*)(struct kref *))kfree);
+	WARN_ON(release == (void (*)(struct kref *))_kfree);
 
 	if (atomic_dec_and_test(&kref->refcount)) {
 		release(kref);

mm/slab.c +2 -2

index 4d00855..3acefac 100644
@@ -3711,7 +3711,7 @@ EXPORT_SYMBOL(kmem_cache_free);
  * Don't free memory not originally allocated by kmalloc()
  * or you will run into trouble.
  */
-void kfree(const void *objp)
+void _kfree(const void *objp)
 {
 	struct kmem_cache *c;
 	unsigned long flags;

@@ -3726,7 +3726,7 @@ void kfree(const void *objp)
 	__cache_free(c, (void *)objp);
 	local_irq_restore(flags);
 }
-EXPORT_SYMBOL(kfree);
+EXPORT_SYMBOL(_kfree);
 
 unsigned int kmem_cache_size(struct kmem_cache *cachep)
 {

mm/slob.c +2 -2

index 52bc8a2..1d8a3c4 100644
@@ -487,7 +487,7 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
 }
 EXPORT_SYMBOL(__kmalloc_node);
 
-void kfree(const void *block)
+void _kfree(const void *block)
 {
 	struct slob_page *sp;

@@ -502,7 +502,7 @@ void kfree(const void *block)
 	} else
 		put_page(&sp->page);
 }
-EXPORT_SYMBOL(kfree);
+EXPORT_SYMBOL(_kfree);
 
 /* can't use ksize for kmem_cache_alloc memory, only kmalloc */
 size_t ksize(const void *block)

mm/slub.c +2 -2

index 0280eee..f9fa9e1 100644
@@ -2738,7 +2738,7 @@ size_t ksize(const void *object)
 }
 EXPORT_SYMBOL(ksize);
 
-void kfree(const void *x)
+void _kfree(const void *x)
 {
 	struct page *page;
 	void *object = (void *)x;

@@ -2754,7 +2754,7 @@ void kfree(const void *x)
 	}
 	slab_free(page->slab, page, object, _RET_IP_);
 }
-EXPORT_SYMBOL(kfree);
+EXPORT_SYMBOL(_kfree);
 
 /*
  * kmem_cache_shrink removes empty slabs from the partial lists and sorts

net/core/dev.c +2 -2

index f112970..3a7a552 100644
@@ -2671,7 +2671,7 @@ void netif_napi_del(struct napi_struct *napi)
 	struct sk_buff *skb, *next;
 
 	list_del_init(&napi->dev_list);
-	kfree(napi->skb);
+	kfree_skb(napi->skb);
 
 	for (skb = napi->gro_list; skb; skb = next) {
 		next = skb->next;

@@ -4720,7 +4720,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	if (!tx) {
 		printk(KERN_ERR "alloc_netdev: Unable to allocate "
 		       "tx qdiscs.\n");
-		kfree(p);
+		_kfree(p);
 		return NULL;
 	}

security/selinux/ss/services.c +1 -1

index c65e4fe..cefe043 100644
@@ -2862,7 +2862,7 @@ static void security_netlbl_cache_add(struct netlbl_lsm_secattr *secattr,
 	}
 
 	*sid_cache = sid;
-	secattr->cache->free = kfree;
+	secattr->cache->free = _kfree;
 	secattr->cache->data = sid_cache;
 	secattr->flags |= NETLBL_SECATTR_CACHE;
 }

Roel Kluin

author

2009-03-15 16:42 UTC #2 lore raw

Re: build bug on kfree(struct sk_buff*)

I noticed that my initial attempt was invalid. The one below appears to be correct, although I am not sure whether there is a better implementation.

this patch also fixes a kfree(skb) in net/core/dev.c, what do you think?

Roel

Signed-off-by Roel Kluin <[email protected]>

---
 include/linux/kernel.h |    4 ++++
 include/linux/slab.h   |    3 +++
 mm/slab.c              |    2 ++
 mm/slob.c              |    2 ++
 mm/slub.c              |    2 ++
 net/core/dev.c         |    2 +-
 6 files changed, 14 insertions(+), 1 deletions(-)

include/linux/kernel.h +4 -0

index 7fa3718..b7f22d8 100644
@@ -526,6 +526,10 @@ struct sysinfo {
    aren't permitted). */
 #define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
 
+/* If the type of x is TYPE, force a compilation error, otherwise produce x */
+#define BUILD_BUG_ON_TYPE(x, TYPE) __builtin_choose_expr(		\
+		__builtin_types_compatible_p(typeof(x), TYPE), (void)0, (x))
+
 /* Trap pasters of __FUNCTION__ at compile-time */
 #define __FUNCTION__ (__func__)

include/linux/slab.h +3 -0

index 24c5602..30c28f9 100644
@@ -130,6 +130,9 @@ void kfree(const void *);
 void kzfree(const void *);
 size_t ksize(const void *);
 
+/* This ensures that kfree is not called with a struct sk_buff* */
+#define kfree(x) kfree(BUILD_BUG_ON_TYPE(x, struct sk_buff*))
+
 /*
  * Allocator specific definitions. These are mainly used to establish optimized
  * ways to convert kmalloc() calls to kmem_cache_alloc() invocations by

mm/slab.c +2 -0

index 4d00855..cbcd28a 100644
@@ -3711,6 +3711,7 @@ EXPORT_SYMBOL(kmem_cache_free);
  * Don't free memory not originally allocated by kmalloc()
  * or you will run into trouble.
  */
+#undef kfree
 void kfree(const void *objp)
 {
 	struct kmem_cache *c;

@@ -3727,6 +3728,7 @@ void kfree(const void *objp)
 	local_irq_restore(flags);
 }
 EXPORT_SYMBOL(kfree);
+#define kfree(x) kfree(BUILD_BUG_ON_TYPE(x, struct sk_buff*))
 
 unsigned int kmem_cache_size(struct kmem_cache *cachep)
 {

mm/slob.c +2 -0

index 52bc8a2..451325c 100644
@@ -487,6 +487,7 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
 }
 EXPORT_SYMBOL(__kmalloc_node);
 
+#undef kfree
 void kfree(const void *block)
 {
 	struct slob_page *sp;

@@ -503,6 +504,7 @@ void kfree(const void *block)
 		put_page(&sp->page);
 }
 EXPORT_SYMBOL(kfree);
+#define kfree(x) kfree(BUILD_BUG_ON_TYPE(x, struct sk_buff*))
 
 /* can't use ksize for kmem_cache_alloc memory, only kmalloc */
 size_t ksize(const void *block)

mm/slub.c +2 -0

index 0280eee..41943fc 100644
@@ -2738,6 +2738,7 @@ size_t ksize(const void *object)
 }
 EXPORT_SYMBOL(ksize);
 
+#undef kfree
 void kfree(const void *x)
 {
 	struct page *page;

@@ -2755,6 +2756,7 @@ void kfree(const void *x)
 	slab_free(page->slab, page, object, _RET_IP_);
 }
 EXPORT_SYMBOL(kfree);
+#define kfree(x) kfree(BUILD_BUG_ON_TYPE(x, struct sk_buff*))
 
 /*
  * kmem_cache_shrink removes empty slabs from the partial lists and sorts

net/core/dev.c +1 -1

index f112970..c1e9dc0 100644
@@ -2671,7 +2671,7 @@ void netif_napi_del(struct napi_struct *napi)
 	struct sk_buff *skb, *next;
 
 	list_del_init(&napi->dev_list);
-	kfree(napi->skb);
+	kfree_skb(napi->skb);
 
 	for (skb = napi->gro_list; skb; skb = next) {
 		next = skb->next;

Ben Hutchings

2009-03-15 19:33 UTC #3 lore raw

Re: build bug on kfree(struct sk_buff*)

Quoted 4 lines · Roel Kluin

I noticed that my initial attempt was invalid. The one below appears to be correct, although I am not sure whether there is a better implementation.
this patch also fixes a kfree(skb) in net/core/dev.c, what do you think?

[...]

There are many types that should not be allocated and freed using the general-purpose heap functions. Why provide this check only for struct skbuff?

Perhaps you should try to find a general way of detecting this, such as a type annotation for sparse.

Ben.

Signature · 3 lines

Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.