linux-kernel - Re: [RFC PATCH 2/5] can: slcan: remove legacy infrastructure

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:   Mon, 25 Jul 2022 15:09:20 +0200
From:   Max Staudt <max@...as.org>
To:     Dario Binacchi <dario.binacchi@...rulasolutions.com>
Cc:     linux-kernel@...r.kernel.org,
        Jeroen Hofstee <jhofstee@...tronenergy.com>,
        michael@...rulasolutions.com,
        Amarula patchwork <linux-amarula@...rulasolutions.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        Paolo Abeni <pabeni@...hat.com>,
        Vincent Mailhol <mailhol.vincent@...adoo.fr>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [RFC PATCH 2/5] can: slcan: remove legacy infrastructure

On Mon, 25 Jul 2022 08:40:24 +0200
Dario Binacchi <dario.binacchi@...rulasolutions.com> wrote:

> > > @@ -883,72 +786,50 @@ static int slcan_open(struct tty_struct *tty)
> > >       if (!tty->ops->write)
> > >               return -EOPNOTSUPP;
> > >
> > > -     /* RTnetlink lock is misused here to serialize concurrent
> > > -      * opens of slcan channels. There are better ways, but it is
> > > -      * the simplest one.
> > > -      */
> > > -     rtnl_lock();
> > > +     dev = alloc_candev(sizeof(*sl), 1);
> > > +     if (!dev)
> > > +             return -ENFILE;
> > >
> > > -     /* Collect hanged up channels. */
> > > -     slc_sync();
> > > +     sl = netdev_priv(dev);
> > >
> > > -     sl = tty->disc_data;
> > > +     /* Configure TTY interface */
> > > +     tty->receive_room = 65536; /* We don't flow control */
> > > +     sl->rcount   = 0;
> > > +     sl->xleft    = 0;  
> >
> > I suggest moving the zeroing to slc_open() - i.e. to the netdev open
> > function. As a bonus, you can then remove the same two assignments from
> > slc_close() (see above). They are only used when netif_running(), with
> > appropiate guards already in place as far as I can see.  
> 
> I think it is better to keep the code as it is, since at the entry of
> the netdev
> open function, netif_running already returns true (it is set to true by the
> calling function) and therefore it would be less safe to reset the
> rcount and xleft
> fields.

Wow, great catch!

I wonder why __LINK_STATE_START is set before ->ndo_open() is called...?


Since the drivers are similar, I've checked can327. It is unaffected,
because the counters are additionally guarded by a spinlock. Same in
slcan, where netdev_close() takes the spinlock to reset the counters.

So you *could* move them to netdev_open() *if* they are always guarded
by the slcan lock.

Or, leave it as it is, as it seems to be correct. Your choice :)


Thank you!

Max