In fs/udf/inode.c, line 1455, linux 2.6.35, there is the following code:
udfperms = ((inode->i_mode & S_IRWXO)) |
((inode->i_mode & S_IRWXG) << 2) |
((inode->i_mode & S_IRWXU) << 4);
Shouldn't we be shifting by 3 bits? i.e:
udfperms = ((inode->i_mode & S_IRWXO)) |
((inode->i_mode & S_IRWXG) << 3) |
((inode->i_mode & S_IRWXU) << 6);
The S_I.. constants are all defined in include/linux/stat.h as 3-bit values.
I will send a patch if needed.
I code, therefore I am
Alex Davis <[email protected]> writes:
> In fs/udf/inode.c, line 1455, linux 2.6.35, there is the following code:
>
> udfperms = ((inode->i_mode & S_IRWXO)) |
> ((inode->i_mode & S_IRWXG) << 2) |
> ((inode->i_mode & S_IRWXU) << 4);
>
> Shouldn't we be shifting by 3 bits? i.e:
> udfperms = ((inode->i_mode & S_IRWXO)) |
> ((inode->i_mode & S_IRWXG) << 3) |
> ((inode->i_mode & S_IRWXU) << 6);
>
> The S_I.. constants are all defined in include/linux/stat.h as 3-bit values.
>
> I will send a patch if needed.
I would suggest you test it first. Put in a UDF disk that triggers
this case (verify with a printk). Check in ls -l if the
permissions are correct or wrong.
-Andi
--
[email protected] -- Speaking for myself only
Le dimanche 15 mai 2011 à 08:14 -0700, Andi Kleen a écrit :
> Alex Davis <[email protected]> writes:
>
> > In fs/udf/inode.c, line 1455, linux 2.6.35, there is the following code:
> >
> > udfperms = ((inode->i_mode & S_IRWXO)) |
> > ((inode->i_mode & S_IRWXG) << 2) |
> > ((inode->i_mode & S_IRWXU) << 4);
> >
> > Shouldn't we be shifting by 3 bits? i.e:
> > udfperms = ((inode->i_mode & S_IRWXO)) |
> > ((inode->i_mode & S_IRWXG) << 3) |
> > ((inode->i_mode & S_IRWXU) << 6);
> >
> > The S_I.. constants are all defined in include/linux/stat.h as 3-bit values.
> >
> > I will send a patch if needed.
>
> I would suggest you test it first. Put in a UDF disk that triggers
> this case (verify with a printk). Check in ls -l if the
> permissions are correct or wrong.
Well, no need to test ;)
Existing code is fine AFAIK.
fs/udf/ecma_167.h
/* Permissions (ECMA 167r3 4/14.9.5) */
#define FE_PERM_O_EXEC 0x00000001U
#define FE_PERM_O_WRITE 0x00000002U
#define FE_PERM_O_READ 0x00000004U
#define FE_PERM_O_CHATTR 0x00000008U
#define FE_PERM_O_DELETE 0x00000010U
#define FE_PERM_G_EXEC 0x00000020U
#define FE_PERM_G_WRITE 0x00000040U
#define FE_PERM_G_READ 0x00000080U
#define FE_PERM_G_CHATTR 0x00000100U
#define FE_PERM_G_DELETE 0x00000200U
#define FE_PERM_U_EXEC 0x00000400U
#define FE_PERM_U_WRITE 0x00000800U
#define FE_PERM_U_READ 0x00001000U
#define FE_PERM_U_CHATTR 0x00002000U
#define FE_PERM_U_DELETE 0x00004000U
So Other bits (inode->i_mode & S_IRWXO) really maps to
FE_PERM_O_EXEC/WRITE/READ
For Group bits (inode->i_mode & S_IRWXG) we must shift by 2 bits to the
left to make them match FE_PERM_G_EXEC/WRITE/READ (to skip
O_CHATR/O_DELETE)
For Owner/User bits (inode->i_mode & S_IRWXU) we must shift by 4 bits
for same reason.
On 2011-05-15, at 9:14 AM, Andi Kleen <[email protected]> wrote:
> Alex Davis <[email protected]> writes:
>> In fs/udf/inode.c, line 1455, linux 2.6.35, there is the following code:
>>
>> udfperms = ((inode->i_mode & S_IRWXO)) |
>> ((inode->i_mode & S_IRWXG) << 2) |
>> ((inode->i_mode & S_IRWXU) << 4);
>>
>> Shouldn't we be shifting by 3 bits? i.e:
>> udfperms = ((inode->i_mode & S_IRWXO)) |
>> ((inode->i_mode & S_IRWXG) << 3) |
>> ((inode->i_mode & S_IRWXU) << 6);
>>
>> The S_I.. constants are all defined in include/linux/stat.h as 3-bit values.
>>
>> I will send a patch if needed.
>
> I would suggest you test it first. Put in a UDF disk that triggers
> this case (verify with a printk). Check in ls -l if the
> permissions are correct or wrong.
Typically I would agree. In this case ir looks like the existing code doesn't make sense, because it will be overlapping the R and X bits from the adjacent U, G, and O masks.
Cheers, Andreas-
Alex Davis <[email protected]> writes:
> In fs/udf/inode.c, line 1455, linux 2.6.35, there is the following code:
>
> udfperms = ((inode->i_mode & S_IRWXO)) |
> ((inode->i_mode & S_IRWXG) << 2) |
> ((inode->i_mode & S_IRWXU) << 4);
>
> Shouldn't we be shifting by 3 bits? i.e:
> udfperms = ((inode->i_mode & S_IRWXO)) |
> ((inode->i_mode & S_IRWXG) << 3) |
> ((inode->i_mode & S_IRWXU) << 6);
udfperms contains three bit fields of 5 bits each, of which 3 bits are
each filled from one of the three RWX parts of i_mode, and 2 bits
(DELETE and CHATTR) are added later. Thus each of the three bit fields
are expanded from 3 to 5 bits, so that the second one needs to be
shifted by 2 and the third one by 4.
Andreas.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Andi Kleen <[email protected]> writes:
> Alex Davis <[email protected]> writes:
>
>> In fs/udf/inode.c, line 1455, linux 2.6.35, there is the following code:
>>
>> udfperms = ((inode->i_mode & S_IRWXO)) |
>> ((inode->i_mode & S_IRWXG) << 2) |
>> ((inode->i_mode & S_IRWXU) << 4);
>>
>> Shouldn't we be shifting by 3 bits? i.e:
>> udfperms = ((inode->i_mode & S_IRWXO)) |
>> ((inode->i_mode & S_IRWXG) << 3) |
>> ((inode->i_mode & S_IRWXU) << 6);
>>
>> The S_I.. constants are all defined in include/linux/stat.h as 3-bit values.
>>
>> I will send a patch if needed.
>
> I would suggest you test it first. Put in a UDF disk that triggers
> this case (verify with a printk). Check in ls -l if the
> permissions are correct or wrong.
That's the write part of UDF, so a read-only test won't trigger.
Andreas.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
I get it now.
Thanks.
I code, therefore I am
--- On Sun, 5/15/11, Andreas Schwab <[email protected]> wrote:
> From: Andreas Schwab <[email protected]>
> Subject: Re: Possible coding issue in udf??
> To: "Alex Davis" <[email protected]>
> Cc: [email protected], [email protected]
> Date: Sunday, May 15, 2011, 1:13 PM
> Alex Davis <[email protected]>
> writes:
>
> > In fs/udf/inode.c, line 1455, linux 2.6.35, there is
> the following code:
> >
> > udfperms = ((inode->i_mode &
> S_IRWXO)) |
> >
> ((inode->i_mode & S_IRWXG) <<
> 2) |
> >
> ((inode->i_mode & S_IRWXU) <<
> 4);
> >
> > Shouldn't we be shifting by 3 bits? i.e:
> > udfperms = ((inode->i_mode &
> S_IRWXO)) |
> >
> ((inode->i_mode & S_IRWXG) <<
> 3) |
> >
> ((inode->i_mode & S_IRWXU) <<
> 6);
>
> udfperms contains three bit fields of 5 bits each, of which
> 3 bits are
> each filled from one of the three RWX parts of i_mode, and
> 2 bits
> (DELETE and CHATTR) are added later. Thus each of the
> three bit fields
> are expanded from 3 to 5 bits, so that the second one needs
> to be
> shifted by 2 and the third one by 4.
>
> Andreas.
>
> --
> Andreas Schwab, [email protected]
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3
> 44D5 214B 8276 4ED5
> "And now for something completely different."
>