Possible coding issue in udf??

Possible coding issue in udf??

[Alex Davis]

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

Re: Possible coding issue in udf??

[Andi Kleen]

Alex Davis <alex14641@yahoo.com> 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

-- 
ak@linux.intel.com -- Speaking for myself only

Re: Possible coding issue in udf??

[Eric Dumazet]

Le dimanche 15 mai 2011 à 08:14 -0700, Andi Kleen a écrit :
> Alex Davis <alex14641@yahoo.com> 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.

Re: Possible coding issue in udf??

[Andreas Dilger]

On 2011-05-15, at 9:14 AM, Andi Kleen <andi@firstfloor.org> wrote:
> Alex Davis <alex14641@yahoo.com> 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

Re: Possible coding issue in udf??

[Andreas Schwab]

Andi Kleen <andi@firstfloor.org> writes:

> Alex Davis <alex14641@yahoo.com> 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, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Possible coding issue in udf??

[Andreas Schwab]

Alex Davis <alex14641@yahoo.com> 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, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Possible coding issue in udf??

[Alex Davis]

I get it now.

Thanks.

I code, therefore I am


--- On Sun, 5/15/11, Andreas Schwab <schwab@linux-m68k.org> wrote:

> From: Andreas Schwab <schwab@linux-m68k.org>
> Subject: Re: Possible coding issue in udf??
> To: "Alex Davis" <alex14641@yahoo.com>
> Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
> Date: Sunday, May 15, 2011, 1:13 PM
> Alex Davis <alex14641@yahoo.com>
> 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, schwab@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3
> 44D5 214B 8276 4ED5
> "And now for something completely different."
>