linux-kernel - Re: [PATCH 10/30] cr: core stuff

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]

Date:	Fri, 10 Apr 2009 11:35:20 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Alexey Dobriyan <adobriyan@...il.com>
Cc:	akpm@...ux-foundation.org, containers@...ts.linux-foundation.org,
	xemul@...allels.com, serue@...ibm.com, dave@...ux.vnet.ibm.com,
	orenl@...columbia.edu, hch@...radead.org,
	torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/30] cr: core stuff


* Alexey Dobriyan <adobriyan@...il.com> wrote:

> +int cr_restore_file(struct cr_context *ctx, loff_t pos)
> +{

I tried to review this code, but it's almost unreadable to me, due 
to basic code structure mistakes like:

> +	struct cr_image_file *i, *tmp;
> +	struct file *file;
> +	struct cr_object *obj;
> +	char *cr_name;
> +	int rv;
> +
> +	i = kzalloc(sizeof(*i), GFP_KERNEL);
> +	if (!i)
> +		return -ENOMEM;
> +	rv = cr_pread(ctx, i, sizeof(*i), pos);
> +	if (rv < 0) {
> +		kfree(i);
> +		return rv;
> +	}
> +	if (i->cr_hdr.cr_type != CR_OBJ_FILE) {
> +		kfree(i);
> +		return -EINVAL;
> +	}
> +	/* Image of struct file is variable-sized. */
> +	tmp = i;
> +	i = krealloc(i, i->cr_hdr.cr_len + 1, GFP_KERNEL);
> +	if (!i) {
> +		kfree(tmp);
> +		return -ENOMEM;
> +	}
> +	cr_name = (char *)(i + 1);
> +	rv = cr_pread(ctx, cr_name, i->cr_name_len, pos + sizeof(*i));
> +	if (rv < 0) {
> +		kfree(i);
> +		return -ENOMEM;
> +	}
> +	cr_name[i->cr_name_len] = '\0';
> +
> +	file = filp_open(cr_name, i->cr_f_flags, 0);
> +	if (IS_ERR(file)) {
> +		kfree(i);
> +		return PTR_ERR(file);
> +	}
> +	if (file->f_dentry->d_inode->i_mode != i->cr_i_mode) {
> +		fput(file);
> +		kfree(i);
> +		return -EINVAL;
> +	}
> +	if (vfs_llseek(file, i->cr_f_pos, SEEK_SET) != i->cr_f_pos) {
> +		fput(file);
> +		kfree(i);
> +		return -EINVAL;
> +	}
> +
> +	obj = cr_object_create(file);
> +	if (!obj) {
> +		fput(file);
> +		kfree(i);
> +		return -ENOMEM;
> +	}

This contains 7 kfree()s of the same thing (!), 3 fput()s of the 
same thing, replicated all over the place obscuring the real essence 
of the code.

This should be restructured to move all the failure exception cases 
into a clean out of line inverse teardown sequence with proper goto 
labels. That way it will be 70% real code 30% teardown - not 10% 
real code mixed into 90% teardown like above.

Also, whoever named a local variable with a type of
"struct cr_image_file *" as 'i' should be sent back to
coding primary school.

You really should not write new kernel code until you know, follow 
and respect basic code cleanliness principles. I am not inserting 
any more review feedback value into this code until it does not meet 
_basic_ quality standards that make review efforts smooth and 
efficient.

Oh, and then i saw this sequence:

> +	/* Known good and unknown bad flags. */
> +	vm_flags &= ~VM_READ;
> +	vm_flags &= ~VM_WRITE;
> +	vm_flags &= ~VM_EXEC;
> +//	vm_flags &= ~VM_SHARED;
> +	vm_flags &= ~VM_MAYREAD;
> +	vm_flags &= ~VM_MAYWRITE;
> +	vm_flags &= ~VM_MAYEXEC;
> +//	vm_flags &= ~VM_MAYSHARE;
> +	vm_flags &= ~VM_GROWSDOWN;
> +//	vm_flags &= ~VM_GROWSUP;
> +//	vm_flags &= ~VM_PFNMAP;
> +	vm_flags &= ~VM_DENYWRITE;
> +	vm_flags &= ~VM_EXECUTABLE;
> +//	vm_flags &= ~VM_LOCKED;
> +//	vm_flags &= ~VM_IO;
> +//	vm_flags &= ~VM_SEQ_READ;
> +//	vm_flags &= ~VM_RAND_READ;
> +//	vm_flags &= ~VM_DONTCOPY;
> +	vm_flags &= ~VM_DONTEXPAND;
> +//	vm_flags &= ~VM_RESERVED;
> +	vm_flags &= ~VM_ACCOUNT;
> +//	vm_flags &= ~VM_NORESERVE;
> +//	vm_flags &= ~VM_HUGETLB;
> +//	vm_flags &= ~VM_NONLINEAR;
> +//	vm_flags &= ~VM_MAPPED_COPY;
> +//	vm_flags &= ~VM_INSERTPAGE;
> +	vm_flags &= ~VM_ALWAYSDUMP;
> +	vm_flags &= ~VM_CAN_NONLINEAR;
> +//	vm_flags &= ~VM_MIXEDMAP;
> +//	vm_flags &= ~VM_SAO;
> +//	vm_flags &= ~VM_PFN_AT_MMAP;

No comment ...

	Ingo
--
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/