linux-kernel - Re: [patch v2 1/3] tty: add function to convert device name to number

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 18 Jun 2017 16:27:42 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Okash Khawaja <okash.khawaja@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Samuel Thibault <samuel.thibault@...-lyon.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        William Hubbs <w.d.hubbs@...il.com>,
        Chris Brannon <chris@...-brannons.com>,
        Kirk Reiser <kirk@...sers.ca>, speakup@...ux-speakup.org,
        devel@...verdev.osuosl.org
Subject: Re: [patch v2 1/3] tty: add function to convert device name to number

On Sun, Jun 18, 2017 at 11:58 AM, Okash Khawaja <okash.khawaja@...il.com> wrote:
> The function converts strings like ttyS0 and ttyUSB0 to dev_t like
> (4, 64) and (188, 0). It does this by scanning tty_drivers list for
> corresponding device name and index. If the driver is not registered,
> this function returns -ENODEV. It also acquires tty_mutex.

Nice!
Few comments below.

> +/**
> + *     tty_dev_name_to_number  -       return dev_t for device name

> + *     @device_name: user space name of device under /dev

This doesn't have actual parameter name.
Btw, I would drop dev_ suffix completely from parameter (you have it
in function name).

> + *     @dev_no: pointer to dev_t that this function will populate

Here I'm not sure if dev_ prefix is good to have or not.

> + *
> + *     This function converts device names like ttyS0 or ttyUSB1 into dev_t
> + *     like (4, 64) or (188, 1). If no corresponding driver is registered then
> + *     the function returns -ENODEV.
> + *

> + *     Locking: this acquires tty_mutex

...and releases.

Perhaps it makes sense to describe what it protects (like it's done in
some functions around).

> + */
> +int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)

const char *name, right?

> +{
> +       struct tty_driver *p;

> +       int rv, index, prefix_length = 0;

I would keep returned variable on a separate line and name it like
other functions do in this file, i.e. ret.

int ret;

> +       while (!isdigit(*(dev_name + prefix_length)) && prefix_length <
> +                       strlen(dev_name) )
> +               prefix_length++;
> +
> +       if (prefix_length == strlen(dev_name))
> +               return -EINVAL;

Basically, what you need is to get tailing digits, right?

Moreover, there is quite similar piece of code in
tty_find_polling_driver() you may share.

> +       mutex_lock(&tty_mutex);
> +
> +       list_for_each_entry(p, &tty_drivers, tty_drivers)
> +               if (prefix_length == strlen(p->name) && strncmp(dev_name,
> +                                       p->name, prefix_length) == 0) {

> +                       rv = kstrtoint(dev_name + prefix_length, 10, &index);
> +                       if (rv) {

> +                               mutex_unlock(&tty_mutex);
> +                               return rv;

I would go with goto style in this function (since it has locking involved).

> +                       }

All together kstrtoint() is invariant here as far as I can see and can
be done out of locking. Also see above comment how to get line index.

> +                       if (index < p->num) {
> +                               *dev_no = MKDEV(p->major, p->minor_start + index);
> +                               mutex_unlock(&tty_mutex);
> +                               return 0;
> +                       }
> +               }
> +
> +       mutex_unlock(&tty_mutex);
> +       return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(tty_dev_name_to_number);

-- 
With Best Regards,
Andy Shevchenko