On Tue, Feb 12, 2013 at 12:39:34PM +0000, Matt Fleming wrote:
> +static struct dentry *efivarfs_alloc_dentry(struct dentry *parent, char *name)
> +{
> + struct qstr q;
> +
> + q.name = name;
> + q.len = strlen(name);
> +
> + if (efivarfs_d_hash(NULL, NULL, &q))
> + return NULL;
> +
> + return d_alloc(parent, &q);
> +}
> @@ -1098,7 +1177,7 @@ static int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
> if (!inode)
> goto fail_name;
>
> - dentry = d_alloc_name(root, name);
> + dentry = efivarfs_alloc_dentry(root, name);
> if (!dentry)
> goto fail_inode;
Umm... That name has just been built by efivarfs_fill_super() itself, and
AFAICS there's no way for its GUID part to be _not_ lowercase
hex and with proper locations of dashes. So
a) hash value will be exactly full_name_hash(name), unless
efivarfs_valid_name() manages to fail.
b) efivarfs_valid_name() is a serious overkill here - the only things
that might go wrong are length of and dashes in entry->var.VariableName.
Both are trivially checked while we are constructing name (before we do
inode allocation, etc.) and IMO they would be better off there.
IOW, I think this part of the patch is better handled by doing those two
checks several lines before d_alloc_name() and not bothering with
efivarfs_alloc_dentry() at all. I.e.
if (len + 1 + GUID_LEN > NAME_MAX)
goto fail;
name = kmalloc(len + 1 + GUID_LEN + 1, GFP_ATOMIC);
if (!name)
goto fail;
for (i = 0; i < len; i++) {
name[i] = entry->var.VariableName[i] & 0xFF;
if (name[i] == '-')
goto fail_name;
}
and then as in the current efivarfs_fill_super().
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
