Re: [PATCH v5 5/8] fat: restructure export_operations

[Namjae Jeon]

2012/12/5, OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>:
> Namjae Jeon <linkinjeon@xxxxxxxxx> writes:
>
>>>> Let me think, if âsubtreeâ checking is enabled then we should check
>>>> the length condition over here also? Please share if there are any
>>>> other comments also.
>>>
>>> I'm not sure what did you mean. Where is "subtree" check you are
>>> talking? This is fh_to_dentry(), so we don't use parent at all, so
>>> length == 3 is enough?
>> With fileID type="FILEID_FAT_WITHOUT_PARENT", fhlen  should be '3'
>> With fileId type="FILEID_FAT_WITH_PARENT", fhlen should be '5'
>>
>> While encoding, WITH_PARENT is selected when subtree check is enabled
>> on the NFS Server.
>>
>> So, when decoding request is arrived-  fileid type will be among the '2'
>> cases:
>> Now, in case of fh_to_dentry() - when we consider, that the reqquest
>> for fileid type WITH_PARENT
>> then i think the conditions in fh_to_dentry should be:
>>
>> if((fh_type == FILEID_FAT_WITH_PARENT) && fh_len != 5)
>> 	return NULL;
>> else if (fh_len != 3)
>> 	return NULL;
>>
>> So, we took care of these '2' conditions within the switch statement
>> based on the 'fh_type'. We can just change the comparision condition
>> from '<' to '!=':
>> switch (fh_type) {
>> +	case FILEID_FAT_WITHOUT_PARENT:
>> +		if (fh_len != FAT_FID_SIZE_WITHOUT_PARENT)
>> +			return NULL;
>> +	case FILEID_FAT_WITH_PARENT:
>> +		if ((fh_len != FAT_FID_SIZE_WITH_PARENT) &&
>> +			(fh_type == FILEID_FAT_WITH_PARENT))
>> +			return NULL;
>> +		i_pos = fid->i_pos_hi;
>> +		i_pos = (i_pos << 32) | (fid->i_pos_low);
>> +		inode = __fat_nfs_get_inode(sb, 0, fid->i_gen, i_pos);
>> +		break;
>> +	}
>>
>> I think there is no need to push the comparision statements in the
>> begining similar to generic_fh_to_dentry.
>
> I can understand what is doing. I'm asking why there is difference.
>
> 1) generic_fh_to_dentry() allows (*_PARENT && fh_len == 2).
> 2) fat_fh_to_dentry_nostale() doesn't allows (*_PARENT && fh_len == 3).
>
> Why does logic has difference?

When we consider the generic routine of encode_fh() and the structure âfidâ

struct fid {
      struct {
            u32 ino;
            u32 gen;
            u32 parent_ino;
            u32 parent_gen;
      } i32;
};

fh_len= 2(without parent)
fh_len=4(with parent)

Condition checking in export_encode_fh()
{

   if (parent && (len < 4)) {
                *max_len = 4;
                return FILEID_INVALID;
        } else if (len < 2) {
                *max_len = 2;
                return FILEID_INVALID;
        }
        ...
                len = 2;
        ...
                if (parent) {
..
                                len = 4;
                                type = FILEID_INO32_GEN_PARENT;
                }
â
}

The logic does take care of altering the length for the â2â cases
with/without parent.
So, while encoding -> the care has been taken for length checking but
while decoding(generic_fh_to_dentry) the length check is not put in
place.
I think it should be done in the generic routine also.

It should be:
if ((fh_len != 2 && fh_len != 4) ||
                (fh_type != FILEID_INO32_GEN && fh_type !=
FILEID_INO32_GEN_PARENT))
    return NULL;

Please share your opinion.

Thanks.
> --
> OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/