netdev - Re: [PATCH net-next v2] mctp: Add MCTP-over-serial transport binding

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]

Message-ID: <75731fa707871b20444a10c28e9883778fcd6688.camel@codeconstruct.com.au>
Date:   Tue, 23 Nov 2021 16:54:31 +0800
From:   Jeremy Kerr <jk@...econstruct.com.au>
To:     Jiri Slaby <jirislaby@...nel.org>, netdev@...r.kernel.org
Cc:     Matt Johnston <matt@...econstruct.com.au>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH net-next v2] mctp: Add MCTP-over-serial transport binding

Hi Jiri,

Thanks for taking a look at the patch, some comments inline.

> > +       /* ... or the next byte to send is an escaped byte; requiring a
> > +        * single-byte chunk...
> 
> This is not a correct multi-line comment. Or does net/ differ in
> this?

Yep, different for net/:

 https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html#is-the-comment-style-convention-different-for-the-networking-content

(even though I guess we're technically drivers/net/ here...)

> > +static netdev_tx_t mctp_serial_tx(struct sk_buff *skb, struct net_device *ndev)
> > +{
> > +       struct mctp_serial *dev = netdev_priv(ndev);
> > +       unsigned long flags;
> > +
> > +       WARN_ON(dev->txstate != STATE_IDLE);
> > +
> > +       if (skb->len > MCTP_SERIAL_MTU) {
> 
> Shouldn't you implement ndo_change_mtu to forbid larger frames
> instead?

Even better, I can set the dev min and max MTUs in _setup, so we'll get
the netlink extacks reported too. However, I'd still like to leave this
is in to ensure there's no way to overrun the tx buffer.

> > +static void mctp_serial_rx(struct mctp_serial *dev)
> > +{
> > +       struct mctp_skb_cb *cb;
> > +       struct sk_buff *skb;
> > +
> > +       if (dev->rxfcs != dev->rxfcs_rcvd) {
> > +               dev->netdev->stats.rx_dropped++;
> > +               dev->netdev->stats.rx_crc_errors++;
> > +               return;
> > +       }
> > +
> > +       skb = netdev_alloc_skb(dev->netdev, dev->rxlen);
> 
> Just thinking… Wouldn't it be easier to have dev->skb instead of 
> dev->rxbuf and push data to it directly in all those
> mctp_serial_push*()?

There's not a huge benefit to doing so - we still need the RX state
machine, so will still need the mctp_serial_push_* code. Having the
rxbuf means we will always have a buffer available for pushed data,
rather than extra conditions in the rx state machine for if we've failed
to allocate the next skb.

Doing this as a final stage in the rx path means we can keep the state
machine a little simpler, and perform the drop in a single location.

The downside to the current approach is the extra copy in skb_put_data()
though, but we're not really dealing with high-bandwidth links here.

I'm happy to change if there's particular preference either way though!

> > +static int mctp_serial_open(struct tty_struct *tty)
> > +{
> > +       struct mctp_serial *dev;
> > +       struct net_device *ndev;
> > +       char name[32];
> > +       int idx, rc;
> > +
> > +       if (!capable(CAP_NET_ADMIN))
> > +               return -EPERM;
> > +
> > +       if (!tty->ops->write)
> > +               return -EOPNOTSUPP;
> > +
> > +       if (tty->disc_data)
> > +               return -EEXIST;
> 
> This should never happen™.

OK, excellent, I'll remove.

> > +static void mctp_serial_close(struct tty_struct *tty)
> > +{
> > +       struct mctp_serial *dev = tty->disc_data;
> > +       int idx = dev->idx;
> > +
> > +       unregister_netdev(dev->netdev);
> > +       ida_free(&mctp_serial_ida, idx);
> 
> No tx_work flushing/cancelling?

Ooh, yeah. I'll fix in v4.

Cheers,


Jeremy