On Thursday 29 December 2005 03:40, Anderson Lizardo wrote:
Something that sort of caught my eye while looking at this (I generally don't
post here so I'm bitting the bullet and hoping I don't screw up), it seems
that this is an experimental driver, but doesn't contain any sort of uniquely
seperated verbose debug information. Let me try and narrow that down:
> +int mmc_key_instantiate(struct key *key, const void *data, size_t datalen)
> +{
> + struct mmc_key_payload *mpayload;
> + struct device *dev;
> + struct mmc_card *card;
> + int ret;
> +
> + ret = -EINVAL;
> + if (datalen <= 0 || datalen > MMC_KEYLEN_MAXBYTES || !data)
> + goto error;
Right here something about the data being passed to the function is invalid.
> + ret = key_payload_reserve(key, datalen);
> + if (ret < 0)
> + goto error;
> +
> + ret = -ENOMEM;
> + mpayload = kmalloc(sizeof(*mpayload) + datalen, GFP_KERNEL);
> + if (!mpayload)
> + goto error;
Unable to allocate mpayload structure, or something of the like.
> + /* attach the data */
> + mpayload->datalen = datalen;
> + memcpy(mpayload->data, data, datalen);
> + rcu_assign_pointer(key->payload.data, mpayload);
> +
> + ret = -EINVAL;
> + dev = bus_find_device(&mmc_bus_type, NULL, NULL, mmc_match_lockable);
> + if (!dev)
> + goto error;
Unable to locate device.
> + card = dev_to_mmc_card(dev);
> + if (mmc_card_locked(card)) {
> + ret = mmc_lock_unlock(card, key, MMC_LOCK_MODE_UNLOCK);
> + mmc_remove_card(card);
> + mmc_register_card(card);
> + } else
> + ret = mmc_lock_unlock(card, key, MMC_LOCK_MODE_SET_PWD);
> + if (ret)
> + ret = -EKEYREJECTED;
Key was rejected, though I suppose EKEYREJECTED pretty much states that.
[snip snip]
> +
> +/*
> + * update a mmc key
> + * - the key's semaphore is write-locked
> + */
> +int mmc_key_update(struct key *key, const void *data, size_t datalen)
> +{
> + struct mmc_key_payload *mpayload, *zap;
> + struct device *dev;
> + struct mmc_card *card;
> + int ret;
> +
> + ret = -EINVAL;
> + if (datalen <= 0 || datalen > MMC_KEYLEN_MAXBYTES || !data)
> + goto error;
See above about invalid data
> + /* construct a replacement payload */
> + ret = -ENOMEM;
> + mpayload = kmalloc(sizeof(*mpayload) + datalen, GFP_KERNEL);
> + if (!mpayload)
> + goto error;
This code almost seemed similiar to mmc_key_instantiate.. I almost wonder if
the code could be consolidated into a single function with some sort of
update conditional code. With that the debug information wouldn't be
duplicated. so snip
> +#ifdef CONFIG_MMC_PASSWORDS
> + else {
> + ret = register_key_type(&mmc_key_type);
> + if (ret) {
Something about the registration failing.
> + class_unregister(&mmc_host_class);
> + bus_unregister(&mmc_bus_type);
> + }
> + }
> +#endif
> }
> return ret;
> }
> @@ -345,6 +501,9 @@ static void __exit mmc_exit(void)
> {
> class_unregister(&mmc_host_class);
> bus_unregister(&mmc_bus_type);
> +#ifdef CONFIG_MMC_PASSWORDS
> + unregister_key_type(&mmc_key_type);
> +#endif
> }
>
> module_init(mmc_init);
That was mainly it. The verbose debug information is more of a "this would be
nice" sort of thing. Just from a user's perspective of debuggin experimental
drivers, this sort of thing is always nice. The code duplication in
mmc_key_instantiate/update still catches my eye though, there may be a
functional code flow to this that I'm not aware of, so again I bite the
bullet. Best hope that I don't make a fool of myself :).
Chris White
Attachment:
pgpWiBkHN2KXs.pgp
Description: PGP signature
- References:
- [patch 0/5] Add MMC password protection (lock/unlock) support V2
- From: Anderson Lizardo <[email protected]>
- [patch 3/5] Add MMC password protection (lock/unlock) support V2
- From: Anderson Lizardo <[email protected]>
- [patch 0/5] Add MMC password protection (lock/unlock) support V2
- Prev by Date: Re: [patch 1/3] mutex subsystem: trylock
- Next by Date: Re: [PATCH] ipw2200 stack reduction
- Previous by thread: [patch 3/5] Add MMC password protection (lock/unlock) support V2
- Next by thread: [patch 4/5] Add MMC password protection (lock/unlock) support V2
- Index(es):
 |