Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Greg KH <gregkh@linuxfoundation.org>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
	linux-sgx@vger.kernel.org, akpm@linux-foundation.org,
	dave.hansen@intel.com, sean.j.christopherson@intel.com,
	nhorman@redhat.com, npmccallum@redhat.com, serge.ayoun@intel.com,
	shay.katz-zamir@intel.com, haitao.huang@intel.com,
	andriy.shevchenko@linux.intel.com, tglx@linutronix.de,
	kai.svahn@intel.com, bp@alien8.de, josh@joshtriplett.org,
	luto@kernel.org, kai.huang@intel.com, rientjes@google.com,
	cedric.xing@intel.com, puiterwijk@redhat.com,
	linux-security-module@vger.kernel.org,
	Suresh Siddha <suresh.b.siddha@intel.com>
Subject: Re: [PATCH v23 12/24] x86/sgx: Linux Enclave Driver
Date: Thu, 28 Nov 2019 19:24:50 +0100
Message-ID: <20191128182450.GA3493127@kroah.com> (raw)
In-Reply-To: <20191028210324.12475-13-jarkko.sakkinen@linux.intel.com>

On Mon, Oct 28, 2019 at 11:03:12PM +0200, Jarkko Sakkinen wrote:
> +static struct device sgx_encl_dev;

Ugh, really?  After 23 versions of this patchset no one saw this?

> +static struct cdev sgx_encl_cdev;
> +static dev_t sgx_devt;
> +
> +static void sgx_dev_release(struct device *dev)
> +{
> +}

The old kernel documentation used to say I was allowed to make fun of
people who did this, but that was removed as it really wasn't that nice.

But I'm seriously reconsidering that at the moment.

No, this is NOT OK!

Think about what you are doing here, and why you feel that it is ok to
work around a kernel message that was added there explicitly to help you
do things the right way.  I didn't add it just because I felt like it, I
was trying to give you a chance to not get the use of this api
incorrect.

That failed :(

Ugh, not ok.  Seriously, not ok...

> +static __init int sgx_dev_init(const char *name, struct device *dev,
> +			       struct cdev *cdev,
> +			       const struct file_operations *fops, int minor)
> +{
> +	int ret;
> +
> +	device_initialize(dev);

Why do you even need a struct device in the first place?

> +
> +	dev->bus = &sgx_bus_type;
> +	dev->devt = MKDEV(MAJOR(sgx_devt), minor);
> +	dev->release = sgx_dev_release;
> +
> +	ret = dev_set_name(dev, name);
> +	if (ret) {
> +		put_device(dev);
> +		return ret;
> +	}
> +
> +	cdev_init(cdev, fops);

Why a whole cdev?

Why not use a misc device?  YOu only have 2 devices right?  Why not 2
misc devices then?  That saves the use of a whole major number and makes
your code a _LOT_ simpler.

> +	ret = bus_register(&sgx_bus_type);

I'm afraid to look at this bus code.

Instead I'm going to ask, why do you need a bus at all?  What drivers do
you have for this bus?

ugh I don't know why I looked at this code, but it's not ok as-is and
anyone who reviewed the driver model interaction needs to rethink
things...

greg k-h

  parent reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20191028210324.12475-1-jarkko.sakkinen@linux.intel.com>
2019-10-28 21:03 ` Jarkko Sakkinen
2019-10-29  9:29   ` Jarkko Sakkinen
2019-10-30  9:30     ` Sean Christopherson
2019-10-31 21:12       ` Jarkko Sakkinen
2019-11-05 11:11         ` Jarkko Sakkinen
2019-11-08  8:20           ` Jarkko Sakkinen
2019-10-30 13:45   ` Stephen Smalley
2019-10-31 21:17     ` Jarkko Sakkinen
2019-11-01 13:16       ` Stephen Smalley
2019-11-01 13:28         ` Stephen Smalley
2019-11-01 15:32           ` Sean Christopherson
2019-11-01 17:16             ` Stephen Smalley
2019-11-08  8:05               ` Jarkko Sakkinen
2019-11-28 18:24   ` Greg KH [this message]
2019-12-06 20:38     ` Jarkko Sakkinen
2019-12-07  8:09       ` Greg KH
2019-12-09 19:57         ` Jarkko Sakkinen
2019-12-23 11:01           ` Jarkko Sakkinen
2019-10-28 21:03 ` [PATCH v23 15/24] x86/sgx: Add provisioning Jarkko Sakkinen

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191128182450.GA3493127@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=haitao.huang@intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=josh@joshtriplett.org \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=puiterwijk@redhat.com \
    --cc=rientjes@google.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=serge.ayoun@intel.com \
    --cc=shay.katz-zamir@intel.com \
    --cc=suresh.b.siddha@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git