Skip to contentSkip to conversation

Possible coding issue in udf??

DISCUSSION Alex Davis 5 participants 7 messages May 14, 2011 – May 15, 2011 VFS

lore
Share & tools
lore

Alex Davis

author
#0 lore raw

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

#1 lore raw
Quoted 6 lines · 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);
Quoted 8 lines

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

Signature · 1 line
[email protected] -- Speaking for myself only
#2 lore raw
Quoted 13 lines · Andi Kleen
Quoted 11 lines · 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);
Quoted 7 lines
Quoted 3 lines

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.

#3 lore raw
In reply to Andi Kleen
Quoted 12 lines · Andi Kleen
Quoted 11 lines · 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);
Quoted 7 lines
Quoted 3 lines

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

#4 lore raw
In reply to Andi Kleen
Quoted 13 lines · Andi Kleen
Quoted 11 lines · 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);
Quoted 7 lines
Quoted 3 lines

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.

Signature · 3 lines
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
#5 lore raw
In reply to Alex Davis
Quoted 10 lines · 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);

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.

Signature · 3 lines
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
author
#6 lore raw

I get it now.

Thanks.

I code, therefore I am

Quoted 48 lines · Andreas Schwab

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]>

Quoted 1 line · writes:

In fs/udf/inode.c, line 1455, linux 2.6.35, there is

the following code:

Quoted 2 lines
    udfperms = ((inode->i_mode &

S_IRWXO)) |

Quoted 1 line

((inode->i_mode & S_IRWXG) <<
2) |

Quoted 1 line

((inode->i_mode & S_IRWXU) <<
4);

Quoted 3 lines

Shouldn't we be shifting by 3 bits? i.e:

    udfperms = ((inode->i_mode &

S_IRWXO)) |

Quoted 1 line

((inode->i_mode & S_IRWXG) <<
3) |

Quoted 1 line

((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.

Signature · 4 lines
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3
44D5 214B 8276 4ED5
"And now for something completely different."