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

[Geyslan GregÃrio Bem]

2013/10/21 Geyslan GregÃrio Bem <geyslan@xxxxxxxxx>:
> 2013/10/21 Geyslan GregÃrio Bem <geyslan@xxxxxxxxx>:
>> 2013/10/20 Eric Van Hensbergen <ericvh@xxxxxxxxx>:
>>> 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@xxxxxxxxx>
>>> 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@xxxxxxxxxxx>:
>>>> > 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.
>>>> >
>>>> >
>>>
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/