linux-kernel - RE: [PATCHv1 5/7] TAP: Extending tap device create/destroy APIs

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] [day] [month] [year] [list]

Date:   Tue, 17 Jan 2017 23:51:13 +0000
From:   "Grandhi, Sainath" <sainath.grandhi@...el.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "mahesh@...dewar.net" <mahesh@...dewar.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCHv1 5/7] TAP: Extending tap device create/destroy APIs

Please find reply inline.

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@...il.com]
> Sent: Friday, January 06, 2017 3:21 PM
> To: Grandhi, Sainath <sainath.grandhi@...el.com>
> Cc: netdev <netdev@...r.kernel.org>; David S. Miller
> <davem@...emloft.net>; mahesh@...dewar.net; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCHv1 5/7] TAP: Extending tap device create/destroy APIs
> 
> On Sat, Jan 7, 2017 at 12:33 AM, Sainath Grandhi
> <sainath.grandhi@...el.com> wrote:
> > Extending tap APIs get/free_minor and create/destroy_cdev to handle
> > more than one type of virtual interface.
> >
> > Signed-off-by: Sainath Grandhi <sainath.grandhi@...el.com>
> > Tested-by: Sainath Grandhi <sainath.grandhi@...el.com>
> 
> Usually it implies that commiter has tested the stuff.
> 
> > --- a/drivers/net/tap.c
> > +++ b/drivers/net/tap.c
> > @@ -99,12 +99,16 @@ static struct proto tap_proto = {  };
> >
> >  #define TAP_NUM_DEVS (1U << MINORBITS)
> 
> > +
> > +LIST_HEAD(major_list);
> > +
> 
> static ?
Makes sense. Would take care of it.
> 
> > -int tap_get_minor(struct tap_dev *tap)
> > +int tap_get_minor(dev_t major, struct tap_dev *tap)
> >  {
> >         int retval = -ENOMEM;
> > +       struct major_info *tap_major, *tmp;
> > +       bool found = false;
> >
> > -       mutex_lock(&macvtap_major.minor_lock);
> > -       retval = idr_alloc(&macvtap_major.minor_idr, tap, 1, TAP_NUM_DEVS,
> GFP_KERNEL);
> 
> > +       list_for_each_entry_safe(tap_major, tmp, &major_list, next) {
> > +               if (tap_major->major == MAJOR(major)) {
> > +                       found = true;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (!found)
> > +               return -EINVAL;
> 
> This is candidate to be a separate helper function. See also below.
Would define a helper function.
> 
> 
> > -void tap_free_minor(struct tap_dev *tap)
> > +void tap_free_minor(dev_t major, struct tap_dev *tap)
> >  {
> > -       mutex_lock(&macvtap_major.minor_lock);
> > +       struct major_info *tap_major, *tmp;
> 
> > +       bool found = false;
> > +
> > +       list_for_each_entry_safe(tap_major, tmp, &major_list, next) {
> > +               if (tap_major->major == MAJOR(major)) {
> > +                       found = true;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (!found)
> > +               return;
> 
> Here is quite the same code (as above).
> 
> > -static struct tap_dev *dev_get_by_tap_minor(int minor)
> > +static struct tap_dev *dev_get_by_tap_file(int major, int minor)
> >  {
> >         struct net_device *dev = NULL;
> >         struct tap_dev *tap;
> > +       struct major_info *tap_major, *tmp;
> > +       bool found = false;
> >
> > -       mutex_lock(&macvtap_major.minor_lock);
> > -       tap = idr_find(&macvtap_major.minor_idr, minor);
> 
> > +       list_for_each_entry_safe(tap_major, tmp, &major_list, next) {
> > +               if (tap_major->major == major) {
> > +                       found = true;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (!found)
> > +               return NULL;
> 
> And here.
> 
> > +static int tap_list_add(dev_t major, const char *device_name) {
> 
> > +       int err = 0;
> > +       struct major_info *tap_major;
> 
> Perhaps
> +       struct major_info *tap_major;
> +       int err = 0;
> 
> > +
> > +       tap_major = kzalloc(sizeof(*tap_major), GFP_ATOMIC);
> > +
> > +       tap_major->major = MAJOR(major);
> > +
> > +       idr_init(&tap_major->minor_idr);
> > +       mutex_init(&tap_major->minor_lock);
> > +
> > +       tap_major->device_name = device_name;
> > +
> > +       list_add_tail(&tap_major->next, &major_list);
> > +       return err;
> 
> 
> > +       err = tap_list_add(*tap_major, device_name);
> >
> >         return err;
> 
> return tap_list_add();
> 
> >  void tap_destroy_cdev(dev_t major, struct cdev *tap_cdev)  {
> > +       struct major_info *tap_major, *tmp;
> > +       bool found = false;
> > +
> > +       list_for_each_entry_safe(tap_major, tmp, &major_list, next) {
> > +               if (tap_major->major == MAJOR(major)) {
> > +                       found = true;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (!found)
> > +               return;
> 
> And here.
> 
> --
> With Best Regards,
> Andy Shevchenko