linux-kernel - Re: [PATCH] include/linux/slab.h: new KFREE() macro.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1168223734.5259.12.camel@dsl081-166-245.sea1.dsl.speakeasy.net>
Date:	Sun, 07 Jan 2007 18:35:34 -0800
From:	Vadim Lobanov <vlobanov@...akeasy.net>
To:	Amit Choudhary <amit2030@...oo.com>
Cc:	Christoph Hellwig <hch@...radead.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] include/linux/slab.h: new KFREE() macro.

On Sun, 2007-01-07 at 16:02 -0800, Amit Choudhary wrote:
> That's where KFREE(ptr) comes in so that the code doesn't look ugly and still the purpose is
> achieved.

Shoving it into a macro makes it no better.

> > Reading code like that makes me say "wtf?", simply because 'ptr' is not
> > used thereafter,
> 
> Really? Then why do we have all the debugging options to catch re-use of the memory that has been
> freed. So many debugging options has been implemented, so much effort has gone into them, partly
> because programmers sometimes miss correct programming.

Which is exactly why we should continue to let programmers focus on
trying to get their code correct and letting the existing safety tools
catch thinkos, instead of distracting them with confusing and completely
pointless variable assignments.

> I do not know what you are talking about here. You are saying that a function does not need three
> different arrays with different names. How can you say that? How do you know what is the
> requirement?

What I said was that your example proves something entirely different
than what you think: rather than proving the need for your KFREE()
macro, it instead proves the need to design the code correctly from the
start, so as to avoid even thinking about this crud.

If you insist, here's your example function, trivially rewritten without
any NULL assignments. (I used two arrays, not three, to save space. The
basic idea works by-design for any random number of arrays, each of any
arbitrary size.)

struct type1 {
	/* something */
};

struct type2 {
	/* something */
};

#define COUNT 10

void function1(struct type1 **a1, struct type2 **a2, unsigned int sz);

void function2(void)
{
	struct type1 *arr1[COUNT];
	struct type2 *arr2[COUNT];
	int i;

	/* init */
	for (i = 0; i < COUNT; i++) {
		arr1[i] = kmalloc(sizeof(struct type1), ...);
		if (!arr1[i])
			goto free_1;
	}
	for (i = 0; i < COUNT; i++) {
		arr2[i] = kmalloc(sizeof(struct type2), ...);
		if (!arr2[i])
			goto free_2;
	}

	/* work */
	function1(arr1, arr2, COUNT);

	/* fini */
	i = COUNT;
free_2:
	for (i--; i >= 0; i--) {
		kfree(arr2[i]);
	}
	i = COUNT;
free_1:
	for (i--; i >= 0; i--) {
		kfree(arr1[i]);
	}
}

In most cases, though, the above code design would be brain-damaged from
the start: unless the sizes involved are prohibitively large, the
function should be allocating all the memory in a single pass.

So, where's the demonstrated need for KFREE()?

-- Vadim Lobanov

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/