linux-kernel - Re: [PATCH 1/2] eCryptfs: Public key; transport mechanism

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: <20061206215555.85d584ca.akpm@osdl.org>
Date:	Wed, 6 Dec 2006 21:55:55 -0800
From:	Andrew Morton <akpm@...l.org>
To:	Michael Halcrow <mhalcrow@...ibm.com>
Cc:	LKML <linux-kernel@...r.kernel.org>, trevor.highland@...il.com,
	tyhicks@...edu
Subject: Re: [PATCH 1/2] eCryptfs: Public key; transport mechanism

On Wed, 6 Dec 2006 17:06:38 -0600
Michael Halcrow <mhalcrow@...ibm.com> wrote:

> This is a re-submission of the same public key patches (updated for
> 2.6.19-rc6-mm2) that were submitted for review a while back.

I made a number of comments last time around, some temperate, some not.
I trust the temperate ones were addressed?

Is there really no way in which any other kernel subsystem will ever want
functionality of this nature?

> This is the transport code for public key functionality in
> eCryptfs. It manages encryption/decryption request queues with a
> transport mechanism. Currently, netlink is the only implemented
> transport.

I wouldn't view this as an adequate changelog for this sort of work,
frankly.  Not by a long shot.  You've told us very briefly what the patches
do.  You haven't told us why they do it, nor how they do it.

What design decisions went into this?  What options were considered and
eliminated and why?  etc.

It's just a great lump of code dumped in our laps.

>From a quick scan (and I cannot review in more depth because the code is a
complete mystery to this reviewer):

> +	mutex_init(&ecryptfs_msg_ctx_lists_mux);
> +	mutex_lock(&ecryptfs_msg_ctx_lists_mux);

That's a bizarre thing to do.  If there's really any other process which
can take that mutex, the mutex_init() just trashed it.  If there is no
other such process, the mutex_lock() is unneeded.  There should never be
a need to runtime-initialise a static mutex - just use DEFINE_MUTEX.

ecryptfs now has a dependency upon netlink.  There's no CONFIG_NETLINK.  If
CONFIG_NET=n && CONFIG_ECRYPTFS=y is possible, it won't build.

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