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

2 messages, 2 authors, 2012-12-05 · open the first message on its own page

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

From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Date: 2012-12-05 08:56:55

Namjae Jeon [off-list ref] writes:
quoted
quoted
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?
-- 
OGAWA Hirofumi [off-list ref]

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

From: Namjae Jeon <hidden>
Date: 2012-12-05 11:45:40

2012/12/5, OGAWA Hirofumi [off-list ref]:
Namjae Jeon [off-list ref] writes:
quoted
quoted
quoted
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 [off-list ref]
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help