Message-ID: <1337769818.2483.202.camel@sauron.fi.intel.com>
Date:	Wed, 23 May 2012 13:43:38 +0300
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Joel Reardon <joel@...mbassador.com>
Cc:	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] UBIFS: introduce struct ubifs_keymap

Hi,

On Mon, 2012-05-21 at 16:41 +0200, Joel Reardon wrote:
> +/*
> + * This file is part of UBIFS.
> + *
> + * Copyright (C) 2006-2008 Nokia Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc., 51
> + * Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + * Authors: Adrian Hunter
> + *	    Artem Bityutskiy (Битюцкий Артём)
> + */

Please, add another copyright and different author names.

> +
> +/*
> + * The file implements the key storage, assignment, and deletion for UBIFS's
> + * secure deletion enhancement. For more information, read
> + * Documentation/filesystems/ubifsec.txt
> + */
> +
> +#include <linux/random.h>
> +#include "ubifs.h"
> +
> +#define RETURN_VAL_IF(v, e) do { if ((e)) return (v); } while (0)
> +#define RETURN_ON_ERR(e) do { if ((e)) return (e); } while (0)

In the Linux kernel we do not do things like this.

> +static void pos_to_eb_offset(const long long ksa_pos,
> +			     int *ksa_leb, int *ksa_offset)
> +{
> +	*ksa_offset = 0xffffffff & ksa_pos;
> +	*ksa_leb = 0xffffffff & (ksa_pos >> 32);
> +}

Throughout whole UBIFS we use shorter 'offs' for offsets - in variables
and function names. Please, be consistent with that and and use
'ksa_offs'.

> +
> +/**
> + * eb_offset_to_pos - convert a ksa_leb and offset to ksa_pos
> + * @ksa_leb: gets the KSA LEB number for the key position
> + * @ksa_offset: gets the KSA LEB offset for the key position
> + *
> + * This function converts the LEB number and offset for a key into the 64-bit
> + * key position value.
> + */
> +static long long eb_offset_to_pos(int ksa_leb, int ksa_offset)

I suggest different names: 'ksa_pos_couple()' (or 'ksa_pos_join()') and
'ksa_pos_decouple()', how does it sound to you?

> +{
> +	return (((long long) ksa_leb) << 32) + ksa_offset;

Sorry for being that picky, but for some reason in UBIFS we never have a
space between the cast and the identifier.

BTW, cast has higher priority that the shift, so you could have less
brackets, but not sure it would me more readable, so it is up to you.

> +}
> +
> +/**
> + * generate_key - generate a new encryption key
> + * @km: keymap structure to know the key size
> + * @to: key-size-sized buffer to hold the new key.
> + *
> + * This function generates a new random key and places it in the to field. The
> + * keymap stores the size of the key used as a field. We assume that
> + * get_random_bytes() will always yield cryptographically-suitable random
> + * data. If this ever changes, this will need to be updated.
> + */
> +static void generate_key(struct ubifs_keymap *km, u8 *to)
> +{
> +	get_random_bytes(to, km->key_size);
> +}
> +
> +/**
> + * keymap_init - construct and initialize a struct keymap
> + * @c: the ubifs info context to put the keymap into
> + *
> + * This function allocates a keymap data structure and initializes its fields.
> + * The ubifs_info context @c is passed as a parameter and its @km field is set
> + * to the keymap that is preprared by this function. If memory cannot be
> + * allocated, it returns -ENOMEM. Otherwise it returns 0.
> + */
> +int keymap_init(struct ubifs_info *c)
> +{
> +	struct ubifs_keymap *km;
> +
> +	c->km = NULL;

Useless assignment.

> +	km = kmalloc(sizeof(struct ubifs_keymap), GFP_NOFS);
> +	RETURN_VAL_IF(-ENOMEM, !km);

Please, do not do things like this in the linux kernel - we prefer to
open-code error checking for readability. Our eyes are not designed to
read weird constructs like this macro :-) Check how we allocate memory
in UBIFS and do it similarly. In general, try to follow the UBIFS style.

> +
> +	km->key_size = UBIFS_CRYPTO_KEYSIZE;
> +	km->keys_per_leb = c->leb_size / km->key_size;
> +	km->ksa_size = 0;
> +	km->ksa_first = 0;
> +	km->unused = 0;
> +	km->deleted = 0;

Do not set fields to zero - allocate the whole thing with kzalloc()
instead.

> +
> +	c->km = km;
> +	return 0;
> +}
> +
> +/**
> + * keymap_free - destruct and free memory used by a struct keymap
> + * @c: the ubifs info context that contanis the keymap
> + *
> + * This function frees the memory being used by the keymap.
> + */
> +void keymap_free(struct ubifs_info *c)
> +{
> +	kfree(c->km);
> +	c->km = NULL;

Please, kill this helper so far and do not set c->km to NULL. Call
kfree(c->km) directly from whenever you need. If it grows - you can
create a helper later.

> +}

If init/free are called only during 

> +/* keymap.c */
> +int keymap_init(struct ubifs_info *c);
> +void keymap_free(struct ubifs_info *c);
> +
> +/* keymap.c */
> +int keymap_init(struct ubifs_info *c);
> +void keymap_free(struct ubifs_info *c);

2 times the same?

-- 
Best Regards,
Artem Bityutskiy

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists