[V9fs-developer] 9p: unsigned/signed wrap in p9/unix modes.

Commit Message

Geyslan G. Bem Oct. 21, 2013, 9:13 p.m. UTC

2013/10/21 Geyslan Gregório Bem <[e-mail]>:
> 2013/10/20 Eric Van Hensbergen <[e-mail]>:
>> 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 <[e-mail]>
>> 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:

         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 <[e-mail]>:
>>> > 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.
>>> >
>>> >
>>
>>

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from 
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk

Comments

Geyslan G. Bem Oct. 22, 2013, 10:35 a.m. UTC | #1

2013/10/21 Geyslan Gregório Bem <[e-mail]>:
> 2013/10/21 Geyslan Gregório Bem <[e-mail]>:
>> 2013/10/20 Eric Van Hensbergen <[e-mail]>:
>>> 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 <[e-mail]>
>>> 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 <[e-mail]>:
>>>> > 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.
>>>> >
>>>> >
>>>
>>>

------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from 
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk

Eric Van Hensbergen Oct. 27, 2013, 8:09 p.m. UTC | #2

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



On Tue, Oct 22, 2013 at 5:35 AM, Geyslan Gregório Bem <[e-mail]>wrote:

> 2013/10/21 Geyslan Gregório Bem <[e-mail]>:
> > 2013/10/21 Geyslan Gregório Bem <[e-mail]>:
> >> 2013/10/20 Eric Van Hensbergen <[e-mail]>:
> >>> 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 <
> [e-mail]>
> >>> 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 <[e-mail]>:
> >>>> > 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.
> >>>> >
> >>>> >
> >>>
> >>>
>
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from 
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk

Geyslan G. Bem Oct. 28, 2013, 9:20 a.m. UTC | #3

2013/10/27 Eric Van Hensbergen <[e-mail]>

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


>
>
> On Tue, Oct 22, 2013 at 5:35 AM, Geyslan Gregório Bem <[e-mail]>wrote:
>
>> 2013/10/21 Geyslan Gregório Bem <[e-mail]>:
>> > 2013/10/21 Geyslan Gregório Bem <[e-mail]>:
>> >> 2013/10/20 Eric Van Hensbergen <[e-mail]>:
>> >>> 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 <
>> [e-mail]>
>> >>> 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 <[e-mail]>:
>> >>>> > 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.
>> >>>> >
>> >>>> >
>> >>>
>> >>>
>>
>
>

Patch

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