[PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[Geyslan G. Bem]

Changes the sign type to unsigned, avoiding the possibility of
wrap when ORing the p9 or unix bit modes.

Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
---
 fs/9p/vfs_inode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index b352457..574095e 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -63,7 +63,7 @@ static const struct inode_operations v9fs_symlink_inode_operations;
 
 static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
 {
-	int res;
+	u32 res;
 	res = mode & 0777;
 	if (S_ISDIR(mode))
 		res |= P9_DMDIR;
@@ -125,7 +125,7 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
 static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
 			       struct p9_wstat *stat, dev_t *rdev)
 {
-	int res;
+	umode_t res;
 	u32 mode = stat->mode;
 
 	*rdev = 0;
@@ -144,7 +144,7 @@ static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
 	else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
 		 && (v9ses->nodev == 0)) {
 		char type = 0, ext[32];
-		int major = -1, minor = -1;
+		u32 major = 0, minor = 0;
 
 		strlcpy(ext, stat->extension, sizeof(ext));
 		sscanf(ext, "%c %u %u", &type, &major, &minor);
-- 
1.8.4

Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[Joe Perches]

On Mon, 2013-10-07 at 19:19 -0300, Geyslan G. Bem wrote:
> Changes the sign type to unsigned, avoiding the possibility of
> wrap when ORing the p9 or unix bit modes.
[]
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
[]
> @@ -144,7 +144,7 @@ static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
>  	else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
>  		 && (v9ses->nodev == 0)) {
>  		char type = 0, ext[32];
> -		int major = -1, minor = -1;
> +		u32 major = 0, minor = 0;
>  
>  		strlcpy(ext, stat->extension, sizeof(ext));
>  		sscanf(ext, "%c %u %u", &type, &major, &minor);

This bit changes the MKDEV below it.

I would think that the sscanf return should be
checked for 3 and maybe MKDEV should not be
constructed when it's not.

Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[Geyslan Gregório Bem]

Joe,

Thank you for reply.

What do you think about:

                 strncpy(ext, stat->extension, sizeof(ext));
+                 if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) {
+                                  p9_debug(P9_DEBUG_ERROR,
+                                  "It's necessary define type, major
and minor values when using P9_DMDEVICE");
+                                  return res;
+                 }
                 switch (type) {
                 case 'c':
                         res |= S_IFCHR;
                         break;
...
                 *rdev = MKDEV(major, minor);

Geyslan Gregório Bem
hackingbits.com


2013/10/7 Joe Perches <joe@perches.com>:
> On Mon, 2013-10-07 at 19:19 -0300, Geyslan G. Bem wrote:
>> Changes the sign type to unsigned, avoiding the possibility of
>> wrap when ORing the p9 or unix bit modes.
> []
>> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> []
>> @@ -144,7 +144,7 @@ static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
>>       else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
>>                && (v9ses->nodev == 0)) {
>>               char type = 0, ext[32];
>> -             int major = -1, minor = -1;
>> +             u32 major = 0, minor = 0;
>>
>>               strlcpy(ext, stat->extension, sizeof(ext));
>>               sscanf(ext, "%c %u %u", &type, &major, &minor);
>
> This bit changes the MKDEV below it.
>
> I would think that the sscanf return should be
> checked for 3 and maybe MKDEV should not be
> constructed when it's not.
>
>

Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[Joe Perches]

On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote:
> Joe,
> 
> Thank you for reply.
> 
> What do you think about:
> 
>                  strncpy(ext, stat->extension, sizeof(ext));
> +                 if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) {
> +                                  p9_debug(P9_DEBUG_ERROR,
> +                                  "It's necessary define type, major
> and minor values when using P9_DMDEVICE");
> +                                  return res;
> +                 }
>                  switch (type) {
>                  case 'c':
>                          res |= S_IFCHR;
>                          break;
> ...
>                  *rdev = MKDEV(major, minor);

I think the plan 9 folk should figure out what's right.

Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[Geyslan Gregório Bem]

Joe,

Nice, I'll wait their reply, there are other p9 patches that I have
already sent and am awaiting Eric's.

Thank you again.

Geyslan Gregório Bem
hackingbits.com


2013/10/7 Joe Perches <joe@perches.com>:
> On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote:
>> Joe,
>>
>> Thank you for reply.
>>
>> What do you think about:
>>
>>                  strncpy(ext, stat->extension, sizeof(ext));
>> +                 if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) {
>> +                                  p9_debug(P9_DEBUG_ERROR,
>> +                                  "It's necessary define type, major
>> and minor values when using P9_DMDEVICE");
>> +                                  return res;
>> +                 }
>>                  switch (type) {
>>                  case 'c':
>>                          res |= S_IFCHR;
>>                          break;
>> ...
>>                  *rdev = MKDEV(major, minor);
>
> I think the plan 9 folk should figure out what's right.
>
>

Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[Geyslan Gregório Bem]

2013/10/20 Eric Van Hensbergen <ericvh@gmail.com>:
> Please resubmit a clean patch which includes the check of sscanf for exactly
> the correct number of arguments and handles errors properly in other cases.
> That last bit may be a bit problematic since right now the only errors are
> prints and we seem to be otherwise silently failing.  Of course, looks like
> nothing else is checking return values from that function for error.  We
> could set rdev to an ERR_PTR(-EIO), and then go through and do a check in
> all the places which matter (looks like there are a few places where rdev
> just gets discarded -- might even be a good idea to not parse rdev unless we
> need to by passing NULL to p9mode2unixmode.
>
> All in all, its a corner case which is only likely with a broken server, but
> the full clean up would seem to be:
>   a) switch to u32's
>   b) pass NULL when rdev just gets discarded and don't bother parsing when
> it is
>   c) check the sscanf return validity
>   d) on error set ERR_PTR in rdev and check on return before proceeding
>
> That's a lot of cleanup, I'll add it to my work queue if you don't have time
> to rework your patch.
>

Eric, I would like to try with your guidance.

> For the other patches, anyone you didn't see a response from me on today is
> being pulled into my for-next queue.  Thanks for the cleanups.
>
>       -eric

Thanks for accept them.

>
>
>
>
> On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem <geyslan@gmail.com>
> wrote:
>>
>> Joe,
>>
>> Nice, I'll wait their reply, there are other p9 patches that I have
>> already sent and am awaiting Eric's.
>>
>> Thank you again.
>>
>> Geyslan Gregório Bem
>> hackingbits.com
>>
>>
>> 2013/10/7 Joe Perches <joe@perches.com>:
>> > On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote:
>> >> Joe,
>> >>
>> >> Thank you for reply.
>> >>
>> >> What do you think about:
>> >>
>> >>                  strncpy(ext, stat->extension, sizeof(ext));
>> >> +                 if (sscanf(ext, "%c %u %u", &type, &major, &minor) <
>> >> 3) {
>> >> +                                  p9_debug(P9_DEBUG_ERROR,
>> >> +                                  "It's necessary define type, major
>> >> and minor values when using P9_DMDEVICE");
>> >> +                                  return res;
>> >> +                 }
>> >>                  switch (type) {
>> >>                  case 'c':
>> >>                          res |= S_IFCHR;
>> >>                          break;
>> >> ...
>> >>                  *rdev = MKDEV(major, minor);
>> >
>> > I think the plan 9 folk should figure out what's right.
>> >
>> >
>
>

Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[Geyslan Gregório Bem]

2013/10/21 Geyslan Gregório Bem <geyslan@gmail.com>:
> 2013/10/20 Eric Van Hensbergen <ericvh@gmail.com>:
>> Please resubmit a clean patch which includes the check of sscanf for exactly
>> the correct number of arguments and handles errors properly in other cases.
>> That last bit may be a bit problematic since right now the only errors are
>> prints and we seem to be otherwise silently failing.  Of course, looks like
>> nothing else is checking return values from that function for error.  We
>> could set rdev to an ERR_PTR(-EIO), and then go through and do a check in
>> all the places which matter (looks like there are a few places where rdev
>> just gets discarded -- might even be a good idea to not parse rdev unless we
>> need to by passing NULL to p9mode2unixmode.
>>
>> All in all, its a corner case which is only likely with a broken server, but
>> the full clean up would seem to be:
>>   a) switch to u32's
>>   b) pass NULL when rdev just gets discarded and don't bother parsing when
>> it is
>>   c) check the sscanf return validity
>>   d) on error set ERR_PTR in rdev and check on return before proceeding
>>
>> That's a lot of cleanup, I'll add it to my work queue if you don't have time
>> to rework your patch.
>>
>
> Eric, I would like to try with your guidance.
>
>> For the other patches, anyone you didn't see a response from me on today is
>> being pulled into my for-next queue.  Thanks for the cleanups.
>>
>>       -eric
>
> Thanks for accept them.
>
>>
>>
>>
>>
>> On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem <geyslan@gmail.com>
>> wrote:
>>>
>>> Joe,
>>>
>>> Nice, I'll wait their reply, there are other p9 patches that I have
>>> already sent and am awaiting Eric's.
>>>
>>> Thank you again.
>>>
>>> Geyslan Gregório Bem
>>> hackingbits.com
>>>

Let me know if I got your requests:

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 94de6d1..8a332d0 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -63,7 +63,7 @@ static const struct inode_operations
v9fs_symlink_inode_operations;

 static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
 {
-    int res;
+    u32 res;
     res = mode & 0777;
     if (S_ISDIR(mode))
         res |= P9_DMDIR;
@@ -125,7 +125,7 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
 static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
                    struct p9_wstat *stat, dev_t *rdev)
 {
-    int res;
+    umode_t res;
     u32 mode = stat->mode;

     *rdev = 0;
@@ -144,10 +144,15 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
     else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
          && (v9ses->nodev == 0)) {
         char type = 0, ext[32];
-        int major = -1, minor = -1;
+        u32 major = 0, minor = 0;

         strlcpy(ext, stat->extension, sizeof(ext));
-        sscanf(ext, "%c %u %u", &type, &major, &minor);
+        if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) {
+            p9_debug(P9_DEBUG_ERROR,
+                 "It's necessary define type [%u], major [u%] and minor [u%]" \
+                 "values when using P9_DMDEVICE", type, major, minor);
+            goto err_dev;
+        }
         switch (type) {
         case 'c':
             res |= S_IFCHR;
@@ -158,11 +163,16 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
         default:
             p9_debug(P9_DEBUG_ERROR, "Unknown special type %c %s\n",
                  type, stat->extension);
+            goto err_dev;
         };
         *rdev = MKDEV(major, minor);
     } else
         res |= S_IFREG;
+    goto ret;

+err_dev:
+    rdev = ERR_PTR(-EIO);
+ret:
     return res;
 }

@@ -460,13 +470,17 @@ void v9fs_evict_inode(struct inode *inode)

 static int v9fs_test_inode(struct inode *inode, void *data)
 {
-    int umode;
+    umode_t umode;
     dev_t rdev;
     struct v9fs_inode *v9inode = V9FS_I(inode);
     struct p9_wstat *st = (struct p9_wstat *)data;
     struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);

     umode = p9mode2unixmode(v9ses, st, &rdev);
+
+    if (IS_ERR(rdev))
+        return 0;
+
     /* don't match inode of different type */
     if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
         return 0;
@@ -526,6 +540,11 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
      */
     inode->i_ino = i_ino;
     umode = p9mode2unixmode(v9ses, st, &rdev);
+    if (IS_ERR(rdev)) {
+        retval = rdev;
+        goto error;
+    }
+
     retval = v9fs_init_inode(v9ses, inode, umode, rdev);
     if (retval)
         goto error;
@@ -1461,9 +1480,10 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
*dentry, umode_t mode, dev_t rde

 int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode)
 {
-    int umode;
+    umode_t umode;
     dev_t rdev;
     loff_t i_size;
+    int ret = 0;
     struct p9_wstat *st;
     struct v9fs_session_info *v9ses;

@@ -1475,6 +1495,11 @@ int v9fs_refresh_inode(struct p9_fid *fid,
struct inode *inode)
      * Don't update inode if the file type is different
      */
     umode = p9mode2unixmode(v9ses, st, &rdev);
+    if (IS_ERR(rdev)) {
+        ret = rdev;
+        goto out;
+    }
+
     if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
         goto out;

@@ -1491,7 +1516,7 @@ int v9fs_refresh_inode(struct p9_fid *fid,
struct inode *inode)
 out:
     p9stat_free(st);
     kfree(st);
-    return 0;
+    return ret;
 }

 static const struct inode_operations v9fs_dir_inode_operations_dotu = {


>>>
>>> 2013/10/7 Joe Perches <joe@perches.com>:
>>> > On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote:
>>> >> Joe,
>>> >>
>>> >> Thank you for reply.
>>> >>
>>> >> What do you think about:
>>> >>
>>> >>                  strncpy(ext, stat->extension, sizeof(ext));
>>> >> +                 if (sscanf(ext, "%c %u %u", &type, &major, &minor) <
>>> >> 3) {
>>> >> +                                  p9_debug(P9_DEBUG_ERROR,
>>> >> +                                  "It's necessary define type, major
>>> >> and minor values when using P9_DMDEVICE");
>>> >> +                                  return res;
>>> >> +                 }
>>> >>                  switch (type) {
>>> >>                  case 'c':
>>> >>                          res |= S_IFCHR;
>>> >>                          break;
>>> >> ...
>>> >>                  *rdev = MKDEV(major, minor);
>>> >
>>> > I think the plan 9 folk should figure out what's right.
>>> >
>>> >
>>
>>

Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[Geyslan Gregório Bem]

2013/10/21 Geyslan Gregório Bem <geyslan@gmail.com>:
> 2013/10/21 Geyslan Gregório Bem <geyslan@gmail.com>:
>> 2013/10/20 Eric Van Hensbergen <ericvh@gmail.com>:
>>> Please resubmit a clean patch which includes the check of sscanf for exactly
>>> the correct number of arguments and handles errors properly in other cases.
>>> That last bit may be a bit problematic since right now the only errors are
>>> prints and we seem to be otherwise silently failing.  Of course, looks like
>>> nothing else is checking return values from that function for error.  We
>>> could set rdev to an ERR_PTR(-EIO), and then go through and do a check in
>>> all the places which matter (looks like there are a few places where rdev
>>> just gets discarded -- might even be a good idea to not parse rdev unless we
>>> need to by passing NULL to p9mode2unixmode.
>>>
>>> All in all, its a corner case which is only likely with a broken server, but
>>> the full clean up would seem to be:
>>>   a) switch to u32's
>>>   b) pass NULL when rdev just gets discarded and don't bother parsing when
>>> it is
>>>   c) check the sscanf return validity
>>>   d) on error set ERR_PTR in rdev and check on return before proceeding
>>>
>>> That's a lot of cleanup, I'll add it to my work queue if you don't have time
>>> to rework your patch.
>>>
>>
>> Eric, I would like to try with your guidance.
>>
>>> For the other patches, anyone you didn't see a response from me on today is
>>> being pulled into my for-next queue.  Thanks for the cleanups.
>>>
>>>       -eric
>>
>> Thanks for accept them.
>>
>>>
>>>
>>>
>>>
>>> On Mon, Oct 7, 2013 at 7:18 PM, Geyslan Gregório Bem <geyslan@gmail.com>
>>> wrote:
>>>>
>>>> Joe,
>>>>
>>>> Nice, I'll wait their reply, there are other p9 patches that I have
>>>> already sent and am awaiting Eric's.
>>>>
>>>> Thank you again.
>>>>
>>>> Geyslan Gregório Bem
>>>> hackingbits.com
>>>>
>
> Let me know if I got your requests:
>
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 94de6d1..8a332d0 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -63,7 +63,7 @@ static const struct inode_operations
> v9fs_symlink_inode_operations;
>
>  static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
>  {
> -    int res;
> +    u32 res;
>      res = mode & 0777;
>      if (S_ISDIR(mode))
>          res |= P9_DMDIR;
> @@ -125,7 +125,7 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
>  static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
>                     struct p9_wstat *stat, dev_t *rdev)
>  {
> -    int res;
> +    umode_t res;
>      u32 mode = stat->mode;
>
>      *rdev = 0;
> @@ -144,10 +144,15 @@ static umode_t p9mode2unixmode(struct
> v9fs_session_info *v9ses,
>      else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
>           && (v9ses->nodev == 0)) {
>          char type = 0, ext[32];
> -        int major = -1, minor = -1;
> +        u32 major = 0, minor = 0;
>
>          strlcpy(ext, stat->extension, sizeof(ext));
> -        sscanf(ext, "%c %u %u", &type, &major, &minor);
> +        if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) {
> +            p9_debug(P9_DEBUG_ERROR,
> +                 "It's necessary define type [%u], major [u%] and minor [u%]" \
> +                 "values when using P9_DMDEVICE", type, major, minor);
> +            goto err_dev;
> +        }
>          switch (type) {
>          case 'c':
>              res |= S_IFCHR;
> @@ -158,11 +163,16 @@ static umode_t p9mode2unixmode(struct
> v9fs_session_info *v9ses,
>          default:
>              p9_debug(P9_DEBUG_ERROR, "Unknown special type %c %s\n",
>                   type, stat->extension);
> +            goto err_dev;
>          };
>          *rdev = MKDEV(major, minor);
>      } else
>          res |= S_IFREG;
> +    goto ret;
>
> +err_dev:
> +    rdev = ERR_PTR(-EIO);
> +ret:
>      return res;
>  }
>
> @@ -460,13 +470,17 @@ void v9fs_evict_inode(struct inode *inode)
>
>  static int v9fs_test_inode(struct inode *inode, void *data)
>  {
> -    int umode;
> +    umode_t umode;
>      dev_t rdev;
>      struct v9fs_inode *v9inode = V9FS_I(inode);
>      struct p9_wstat *st = (struct p9_wstat *)data;
>      struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
>
>      umode = p9mode2unixmode(v9ses, st, &rdev);
> +
> +    if (IS_ERR(rdev))
> +        return 0;
> +
>      /* don't match inode of different type */
>      if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
>          return 0;
> @@ -526,6 +540,11 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
>       */
>      inode->i_ino = i_ino;
>      umode = p9mode2unixmode(v9ses, st, &rdev);
> +    if (IS_ERR(rdev)) {
> +        retval = rdev;
> +        goto error;
> +    }
> +
>      retval = v9fs_init_inode(v9ses, inode, umode, rdev);
>      if (retval)
>          goto error;
> @@ -1461,9 +1480,10 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
> *dentry, umode_t mode, dev_t rde
>
>  int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode)
>  {
> -    int umode;
> +    umode_t umode;
>      dev_t rdev;
>      loff_t i_size;
> +    int ret = 0;
>      struct p9_wstat *st;
>      struct v9fs_session_info *v9ses;
>
> @@ -1475,6 +1495,11 @@ int v9fs_refresh_inode(struct p9_fid *fid,
> struct inode *inode)
>       * Don't update inode if the file type is different
>       */
>      umode = p9mode2unixmode(v9ses, st, &rdev);
> +    if (IS_ERR(rdev)) {
> +        ret = rdev;
> +        goto out;
> +    }
> +
>      if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
>          goto out;
>
> @@ -1491,7 +1516,7 @@ int v9fs_refresh_inode(struct p9_fid *fid,
> struct inode *inode)
>  out:
>      p9stat_free(st);
>      kfree(st);
> -    return 0;
> +    return ret;
>  }
>
>  static const struct inode_operations v9fs_dir_inode_operations_dotu = {
>

I didn't compile it. It's just a sketch.

The rdev tests (in the callers) must be:
if (rdev < 0) {
...

Waiting your reply.

Cheers.
>
>>>>
>>>> 2013/10/7 Joe Perches <joe@perches.com>:
>>>> > On Mon, 2013-10-07 at 21:09 -0300, Geyslan Gregório Bem wrote:
>>>> >> Joe,
>>>> >>
>>>> >> Thank you for reply.
>>>> >>
>>>> >> What do you think about:
>>>> >>
>>>> >>                  strncpy(ext, stat->extension, sizeof(ext));
>>>> >> +                 if (sscanf(ext, "%c %u %u", &type, &major, &minor) <
>>>> >> 3) {
>>>> >> +                                  p9_debug(P9_DEBUG_ERROR,
>>>> >> +                                  "It's necessary define type, major
>>>> >> and minor values when using P9_DMDEVICE");
>>>> >> +                                  return res;
>>>> >> +                 }
>>>> >>                  switch (type) {
>>>> >>                  case 'c':
>>>> >>                          res |= S_IFCHR;
>>>> >>                          break;
>>>> >> ...
>>>> >>                  *rdev = MKDEV(major, minor);
>>>> >
>>>> > I think the plan 9 folk should figure out what's right.
>>>> >
>>>> >
>>>
>>>

Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[Geyslan Gregório Bem]

2013/10/28 Geyslan Gregório Bem <geyslan@gmail.com>
>
> 2013/10/27 Eric Van Hensbergen <ericvh@gmail.com>
>>
>> Looks like the right approach.  The one other optional thing I mentioned was support for passing NULL for rdev and not trying to parse the device info when rdev == NULL.  Its a very slight optimization in the grand scheme of things, but would seem to be cleaner for the folks calling the function who don't touch rdev after the fact...
>>
>>      -eric
>>
> Great. Let me do the changes this afternoon.
>
>
Hi Eric and all.

You requested to avoid the parsing of device when rdev is NULL, all
right? But I'm afraid that that manner the res (return value) can be
returned wrong when the bit mode is a device. Well, I did some
changes. In this new approach, when rdev is NULL, the function only
doesn't make the device, but returns the res (umode_t) nicely.

Tell me if this approach is correct. Do I have to modify something else?

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 94de6d1..e3d56f1 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -63,7 +63,7 @@ static const struct inode_operations
v9fs_symlink_inode_operations;

 static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
 {
-    int res;
+    u32 res;
     res = mode & 0777;
     if (S_ISDIR(mode))
         res |= P9_DMDIR;
@@ -125,10 +125,9 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
 static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
                    struct p9_wstat *stat, dev_t *rdev)
 {
-    int res;
+    umode_t res;
     u32 mode = stat->mode;

-    *rdev = 0;
     res = p9mode2perm(v9ses, stat);

     if ((mode & P9_DMDIR) == P9_DMDIR)
@@ -144,10 +143,15 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
     else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
          && (v9ses->nodev == 0)) {
         char type = 0, ext[32];
-        int major = -1, minor = -1;
+        u32 major = 0, minor = 0;

         strlcpy(ext, stat->extension, sizeof(ext));
-        sscanf(ext, "%c %u %u", &type, &major, &minor);
+        if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) {
+            p9_debug(P9_DEBUG_ERROR,
+                 "It's necessary define type [%c], major [%u] and minor [%u]" \
+                 "values when mode is P9_DMDEVICE\n", type, major, minor);
+            goto err_dev;
+        }
         switch (type) {
         case 'c':
             res |= S_IFCHR;
@@ -158,11 +162,18 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
         default:
             p9_debug(P9_DEBUG_ERROR, "Unknown special type %c %s\n",
                  type, stat->extension);
-        };
-        *rdev = MKDEV(major, minor);
+            goto err_dev;
+        }
+        /* Only make device if rdev pointer isn't NULL */
+        if (rdev)
+            *rdev = MKDEV(major, minor);
     } else
         res |= S_IFREG;
-
+    goto ret;
+err_dev:
+    if (rdev)
+        rdev = ERR_PTR(-EIO);
+ret:
     return res;
 }

@@ -460,13 +471,12 @@ void v9fs_evict_inode(struct inode *inode)

 static int v9fs_test_inode(struct inode *inode, void *data)
 {
-    int umode;
-    dev_t rdev;
+    umode_t umode;
     struct v9fs_inode *v9inode = V9FS_I(inode);
     struct p9_wstat *st = (struct p9_wstat *)data;
     struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);

-    umode = p9mode2unixmode(v9ses, st, &rdev);
+    umode = p9mode2unixmode(v9ses, st, NULL);
     /* don't match inode of different type */
     if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
         return 0;
@@ -526,6 +536,10 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
      */
     inode->i_ino = i_ino;
     umode = p9mode2unixmode(v9ses, st, &rdev);
+    if (IS_ERR_VALUE(rdev)) {
+        retval = rdev;
+        goto error;
+    }
     retval = v9fs_init_inode(v9ses, inode, umode, rdev);
     if (retval)
         goto error;
@@ -1461,8 +1475,7 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
*dentry, umode_t mode, dev_t rde

 int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode)
 {
-    int umode;
-    dev_t rdev;
+    umode_t umode;
     loff_t i_size;
     struct p9_wstat *st;
     struct v9fs_session_info *v9ses;
@@ -1474,7 +1487,7 @@ int v9fs_refresh_inode(struct p9_fid *fid,
struct inode *inode)
     /*
      * Don't update inode if the file type is different
      */
-    umode = p9mode2unixmode(v9ses, st, &rdev);
+    umode = p9mode2unixmode(v9ses, st, NULL);
     if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
         goto out;


-- 
Regards,

Geyslan G. Bem
hackingbits.com

Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.

[Geyslan Gregório Bem]

2013/10/28 Geyslan Gregório Bem <geyslan@gmail.com>:
> 2013/10/28 Geyslan Gregório Bem <geyslan@gmail.com>
>>
>> 2013/10/27 Eric Van Hensbergen <ericvh@gmail.com>
>>>
>>> Looks like the right approach.  The one other optional thing I mentioned was support for passing NULL for rdev and not trying to parse the device info when rdev == NULL.  Its a very slight optimization in the grand scheme of things, but would seem to be cleaner for the folks calling the function who don't touch rdev after the fact...
>>>
>>>      -eric
>>>
>> Great. Let me do the changes this afternoon.
>>
>>
> Hi Eric and all.
>
> You requested to avoid the parsing of device when rdev is NULL, all
> right? But I'm afraid that that manner the res (return value) can be
> returned wrong when the bit mode is a device. Well, I did some
> changes. In this new approach, when rdev is NULL, the function only
> doesn't make the device, but returns the res (umode_t) nicely.
>
> Tell me if this approach is correct. Do I have to modify something else?
>
> --
> Regards,
>
> Geyslan G. Bem
> hackingbits.com

Eric, I sent the new patch:
[PATCH] 9p: code refactor in vfs_inode.c

-- 
Regards,

Geyslan G. Bem
hackingbits.com