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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20120827064920.GA5170@kroah.com>
Date:	Sun, 26 Aug 2012 23:49:20 -0700
From:	Greg KH <gregkh@...uxfoundation.org>
To:	Arun Murthy <arun.murthy@...ricsson.com>
Cc:	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, alan@...rguk.ukuu.org.uk
Subject: Re: [PATCHv3 1/4] modem_shm: Add Modem Access Framework

On Mon, Aug 27, 2012 at 11:19:26AM +0530, Arun Murthy wrote:
> +struct modem {
> +	struct device *dev;

No, this needs to be:
	struct device dev;

Please fix it up to be like that, otherwise you are not using the driver
model properly at all.

> +/**
> + * modem_register - register a modem
> + * @modem_desc: - description for modem
> + * @dev:        - device
> + * @driver_data:- driver specific data
> + *
> + * Register a modem with the modem access framework, so that
> + * it could be used by client drivers for accessing the
> + * modem.
> + */
> +struct modem_dev *modem_register(struct modem_desc *modem_desc,
> +		struct device *dev,
> +		void *driver_data)
> +{
> +	static atomic_t modem_no = ATOMIC_INIT(0);
> +	struct modem_dev *mdev;
> +	int ret;
> +
> +	if (modem_desc == NULL)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (modem_desc->name == NULL || modem_desc->ops == NULL)
> +		return ERR_PTR(-EINVAL);
> +
> +	mdev = kzalloc(sizeof(struct modem_dev), GFP_KERNEL);
> +	if (mdev == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mutex_lock(&modem_list_mutex);
> +
> +	mutex_init(&mdev->mutex);
> +	mdev->modem_data = driver_data;
> +	mdev->owner = modem_desc->owner;
> +	mdev->desc = modem_desc;
> +	INIT_LIST_HEAD(&mdev->client_list);
> +	INIT_LIST_HEAD(&mdev->modem_list);
> +	BLOCKING_INIT_NOTIFIER_HEAD(&mdev->notifier);
> +
> +	/* mdev->dev.class = &modem_class;*/
> +	mdev->dev.parent = dev;
> +	dev_set_name(&mdev->dev, "modem.%d", atomic_inc_return(&modem_no) - 1);

Use the idr interface instead please.

> +	ret = device_register(&mdev->dev);
> +	if (ret != 0)
> +		goto clean;
> +
> +	dev_set_drvdata(&mdev->dev, mdev);
> +
> +	ret = add_modem_attributes(mdev);
> +	if (ret < 0)
> +		goto backoff;
> +
> +	list_add(&mdev->modem_list, &modem_list);

Hint, you don't need such a list if you use the driver model properly...

> +out:
> +	mutex_unlock(&modem_list_mutex);
> +	return mdev;
> +
> +backoff:
> +	device_unregister(&mdev->dev);
> +	mdev = ERR_PTR(ret);
> +	goto out;
> +
> +clean:
> +	kfree(mdev);
> +	mdev = ERR_PTR(ret);
> +	goto out;
> +}
> +EXPORT_SYMBOL_GPL(modem_register);

You have no "modem_unregister()" call, which, if you did, and actually
tested it out, would show you the problem in the code above (hint, you
really didn't read the documentation I pointed you at previously.)

You are also racing userspace with your device creation, and I can't
tell the difference between a modem_dev and a modem, why have two
different things?

This whole thing needs to be totally reworked, start with the above
comments, and you will see whole sections of this patch just disappear,
as you properly start to use the driver model.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ