Clarify memory/string comparisons
[elmcan.git] / module / elmcan.c
index 95a82069dd6740793dba256a0dbcb304c9d34f34..35f194e561a70e6de46869181d2a885903811e15 100644 (file)
@@ -96,15 +96,6 @@ struct elmcan {
        /* Per-channel lock */
        spinlock_t lock;
 
-       /* Keep track of how many things are using this struct.
-        * Once it reaches 0, we are in the process of cleaning up,
-        * and new operations will be cancelled immediately.
-        * Use atomic_t rather than refcount_t because we deliberately
-        * decrement to 0, and refcount_dec() spills a WARN_ONCE in
-        * that case.
-        */
-       atomic_t refcount;
-
        /* Stop the channel on hardware failure.
         * Once this is true, nothing will be sent to the TTY.
         */
@@ -112,12 +103,12 @@ struct elmcan {
 
        /* TTY TX helpers */
        struct work_struct tx_work;     /* Flushes TTY TX buffer   */
-       unsigned char *txbuf;
-       unsigned char *txhead;          /* Pointer to next TX byte */
-       int txleft;                     /* Bytes left to TX */
+       u8 *txbuf;                      /* Pointer to our TX buffer */
+       u8 *txhead;                     /* Pointer to next TX byte */
+       unsigned txleft;                /* Bytes left to TX */
 
        /* TTY RX helpers */
-       unsigned char rxbuf[ELM327_SIZE_RXBUF];
+       u8 rxbuf[ELM327_SIZE_RXBUF];
        int rxfill;
 
        /* State machine */
@@ -142,18 +133,11 @@ struct elmcan {
        unsigned long cmds_todo;
 };
 
-/* A lock for all tty->disc_data handled by this ldisc.
- * This is to prevent a case where tty->disc_data is set to NULL,
- * yet someone is still trying to dereference it.
- * Without this, we cannot do a clean shutdown.
- */
-static DEFINE_SPINLOCK(elmcan_discdata_lock);
-
 static inline void elm327_hw_failure(struct elmcan *elm);
 
 static void elm327_send(struct elmcan *elm, const void *buf, size_t len)
 {
-       int actual;
+       int written;
 
        lockdep_assert_held(elm->lock);
 
@@ -162,17 +146,8 @@ static void elm327_send(struct elmcan *elm, const void *buf, size_t len)
 
        memcpy(elm->txbuf, buf, len);
 
-       /* Order of next two lines is *very* important.
-        * When we are sending a little amount of data,
-        * the transfer may be completed inside the ops->write()
-        * routine, because it's running with interrupts enabled.
-        * In this case we *never* got WRITE_WAKEUP event,
-        * if we did not request it before write operation.
-        *       14 Oct 1994  Dmitry Gorodchanin.
-        */
-       set_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
-       actual = elm->tty->ops->write(elm->tty, elm->txbuf, len);
-       if (actual < 0) {
+       written = elm->tty->ops->write(elm->tty, elm->txbuf, len);
+       if (written < 0) {
                netdev_err(elm->dev,
                           "Failed to write to tty %s.\n",
                           elm->tty->name);
@@ -180,8 +155,13 @@ static void elm327_send(struct elmcan *elm, const void *buf, size_t len)
                return;
        }
 
-       elm->txleft = len - actual;
-       elm->txhead = elm->txbuf + actual;
+       elm->txleft = len - written;
+       elm->txhead = elm->txbuf + written;
+
+       if (!elm->txleft)
+               netif_wake_queue(elm->dev);
+       else
+               set_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
 }
 
 /* Take the ELM327 out of almost any state and back into command mode.
@@ -336,23 +316,21 @@ static inline void elm327_hw_failure(struct elmcan *elm)
        elm327_feed_frame_to_netdev(elm, skb);
 }
 
-/* Compare a buffer to a fixed string */
-static inline int _memstrcmp(const u8 *mem, const char *str)
-{
-       return memcmp(mem, str, strlen(str));
-}
-
 /* Compare buffer to string length, then compare buffer to fixed string.
  * This ensures two things:
  *  - It flags cases where the fixed string is only the start of the
  *    buffer, rather than exactly all of it.
  *  - It avoids byte comparisons in case the length doesn't match.
+ *
+ * strncmp() cannot be used here because it accepts the following wrong case:
+ *   strncmp("CAN ER", "CAN ERROR", 6);
+ * This must fail, hence this helper function.
  */
-static inline int _len_memstrcmp(const u8 *mem, size_t mem_len, const char *str)
+static inline int check_len_then_cmp(const u8 *mem, size_t mem_len, const char *str)
 {
        size_t str_len = strlen(str);
 
-       return (mem_len != str_len) || memcmp(mem, str, str_len);
+       return (mem_len == str_len) && !memcmp(mem, str, str_len);
 }
 
 static void elm327_parse_error(struct elmcan *elm, size_t len)
@@ -370,29 +348,29 @@ static void elm327_parse_error(struct elmcan *elm, size_t len)
                return;
 
        /* Filter possible error messages based on length of RX'd line */
-       if (!_len_memstrcmp(elm->rxbuf, len, "UNABLE TO CONNECT")) {
+       if (check_len_then_cmp(elm->rxbuf, len, "UNABLE TO CONNECT")) {
                netdev_err(elm->dev,
                           "ELM327 reported UNABLE TO CONNECT. Please check your setup.\n");
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "BUFFER FULL")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "BUFFER FULL")) {
                /* This will only happen if the last data line was complete.
                 * Otherwise, elm327_parse_frame() will heuristically
                 * emit this kind of error frame instead.
                 */
                frame->can_id |= CAN_ERR_CRTL;
                frame->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "BUS ERROR")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "BUS ERROR")) {
                frame->can_id |= CAN_ERR_BUSERROR;
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "CAN ERROR")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "CAN ERROR")) {
                frame->can_id |= CAN_ERR_PROT;
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "<RX ERROR")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "<RX ERROR")) {
                frame->can_id |= CAN_ERR_PROT;
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "BUS BUSY")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "BUS BUSY")) {
                frame->can_id |= CAN_ERR_PROT;
                frame->data[2] = CAN_ERR_PROT_OVERLOAD;
-       } else if (!_len_memstrcmp(elm->rxbuf, len, "FB ERROR")) {
+       } else if (check_len_then_cmp(elm->rxbuf, len, "FB ERROR")) {
                frame->can_id |= CAN_ERR_PROT;
                frame->data[2] = CAN_ERR_PROT_TX;
-       } else if (len == 5 && !_memstrcmp(elm->rxbuf, "ERR")) {
+       } else if (len == 5 && !memcmp(elm->rxbuf, "ERR", 3)) {
                /* ERR is followed by two digits, hence line length 5 */
                netdev_err(elm->dev, "ELM327 reported an ERR%c%c. Please power it off and on again.\n",
                           elm->rxbuf[3], elm->rxbuf[4]);
@@ -509,7 +487,7 @@ static int elm327_parse_frame(struct elmcan *elm, size_t len)
 
        /* Check for RTR frame */
        if (elm->rxfill >= hexlen + 3 &&
-           !_memstrcmp(&elm->rxbuf[hexlen], "RTR")) {
+           !memcmp(&elm->rxbuf[hexlen], "RTR", 3)) {
                frame->can_id |= CAN_RTR_FLAG;
        }
 
@@ -559,7 +537,7 @@ static void elm327_parse_line(struct elmcan *elm, size_t len)
        if (elm->drop_next_line) {
                elm->drop_next_line = 0;
                return;
-       } else if (!_memstrcmp(elm->rxbuf, "AT")) {
+       } else if (!memcmp(elm->rxbuf, "AT", 2)) {
                return;
        }
 
@@ -755,6 +733,7 @@ static void elm327_parse_rxbuf(struct elmcan *elm)
        }
 }
 
+#if LINUX_VERSION_CODE < KERNEL_VERSION(5,10,0)
 /* Dummy needed to use can_rx_offload */
 static struct sk_buff *elmcan_mailbox_read(struct can_rx_offload *offload,
                                           unsigned int n, u32 *timestamp,
@@ -764,6 +743,7 @@ static struct sk_buff *elmcan_mailbox_read(struct can_rx_offload *offload,
 
        return ERR_PTR(-ENOBUFS);
 }
+#endif
 
 static int elmcan_netdev_open(struct net_device *dev)
 {
@@ -792,8 +772,12 @@ static int elmcan_netdev_open(struct net_device *dev)
        elm327_init(elm);
        spin_unlock_bh(&elm->lock);
 
+#if LINUX_VERSION_CODE < KERNEL_VERSION(5,10,0)
        elm->offload.mailbox_read = elmcan_mailbox_read;
        err = can_rx_offload_add_fifo(dev, &elm->offload, ELM327_NAPI_WEIGHT);
+#else
+       err = can_rx_offload_add_manual(dev, &elm->offload, ELM327_NAPI_WEIGHT);
+#endif
        if (err) {
                close_candev(dev);
                return err;
@@ -847,6 +831,9 @@ static netdev_tx_t elmcan_netdev_start_xmit(struct sk_buff *skb,
        struct elmcan *elm = netdev_priv(dev);
        struct can_frame *frame = (struct can_frame *)skb->data;
 
+       if (can_dropped_invalid_skb(dev, skb))
+               return NETDEV_TX_OK;
+
        /* BHs are already disabled, so no spin_lock_bh().
         * See Documentation/networking/netdevices.txt
         */
@@ -886,39 +873,6 @@ static const struct net_device_ops elmcan_netdev_ops = {
        .ndo_change_mtu = can_change_mtu,
 };
 
-/* Get a reference to our struct, taking into account locks/refcounts.
- * This is to ensure ordering in case we are shutting down, and to ensure
- * there is a refcount at all (otherwise tty->disc_data may be freed and
- * before we increment the refcount).
- * Use this for anything that can race against elmcan_ldisc_close().
- */
-static struct elmcan *get_elm(struct tty_struct *tty)
-{
-       struct elmcan *elm;
-       bool got_ref;
-
-       spin_lock_bh(&elmcan_discdata_lock);
-       elm = (struct elmcan *)tty->disc_data;
-
-       if (!elm) {
-               spin_unlock_bh(&elmcan_discdata_lock);
-               return NULL;
-       }
-
-       got_ref = atomic_inc_not_zero(&elm->refcount);
-       spin_unlock_bh(&elmcan_discdata_lock);
-
-       if (!got_ref)
-               return NULL;
-
-       return elm;
-}
-
-static void put_elm(struct elmcan *elm)
-{
-       atomic_dec(&elm->refcount);
-}
-
 static bool elmcan_is_valid_rx_char(char c)
 {
        return (isdigit(c) ||
@@ -947,10 +901,7 @@ static void elmcan_ldisc_rx(struct tty_struct *tty,
                            const unsigned char *cp, const char *fp, int count)
 #endif
 {
-       struct elmcan *elm = get_elm(tty);
-
-       if (!elm)
-               return;
+       struct elmcan *elm = (struct elmcan *)tty->disc_data;
 
        spin_lock_bh(&elm->lock);
 
@@ -1002,7 +953,6 @@ static void elmcan_ldisc_rx(struct tty_struct *tty,
 
 out:
        spin_unlock_bh(&elm->lock);
-       put_elm(elm);
 }
 
 /* Write out remaining transmit buffer.
@@ -1010,59 +960,44 @@ out:
  */
 static void elmcan_ldisc_tx_worker(struct work_struct *work)
 {
-       /* No need to use get_elm() here, as we'll always flush workers
-        * before destroying the elmcan object.
-        */
        struct elmcan *elm = container_of(work, struct elmcan, tx_work);
-       ssize_t actual;
+       ssize_t written;
 
-       spin_lock_bh(&elm->lock);
-       if (elm->hw_failure) {
-               spin_unlock_bh(&elm->lock);
+       if (elm->hw_failure)
                return;
-       }
 
-       if (!elm->tty || !netif_running(elm->dev)) {
-               spin_unlock_bh(&elm->lock);
-               return;
+       spin_lock_bh(&elm->lock);
+
+       if (elm->txleft) {
+               written = elm->tty->ops->write(elm->tty, elm->txhead, elm->txleft);
+               if (written < 0) {
+                       netdev_err(elm->dev,
+                                  "Failed to write to tty %s.\n",
+                                  elm->tty->name);
+                       elm327_hw_failure(elm);
+                       spin_unlock_bh(&elm->lock);
+                       return;
+               } else {
+                       elm->txleft -= written;
+                       elm->txhead += written;
+               }
        }
 
-       if (elm->txleft <= 0)  {
-               /* Our TTY write buffer is empty:
-                * Allow netdev to hand us another packet
-                */
+       if (!elm->txleft)  {
                clear_bit(TTY_DO_WRITE_WAKEUP, &elm->tty->flags);
                spin_unlock_bh(&elm->lock);
                netif_wake_queue(elm->dev);
-               return;
-       }
-
-       actual = elm->tty->ops->write(elm->tty, elm->txhead, elm->txleft);
-       if (actual < 0) {
-               netdev_err(elm->dev,
-                          "Failed to write to tty %s.\n",
-                          elm->tty->name);
-               elm327_hw_failure(elm);
+       } else {
                spin_unlock_bh(&elm->lock);
-               return;
        }
-
-       elm->txleft -= actual;
-       elm->txhead += actual;
-       spin_unlock_bh(&elm->lock);
 }
 
 /* Called by the driver when there's room for more data. */
 static void elmcan_ldisc_tx_wakeup(struct tty_struct *tty)
 {
-       struct elmcan *elm = get_elm(tty);
-
-       if (!elm)
-               return;
+       struct elmcan *elm = (struct elmcan *)tty->disc_data;
 
        schedule_work(&elm->tx_work);
-
-       put_elm(elm);
 }
 
 /* ELM327 can only handle bitrates that are integer divisors of 500 kHz,
@@ -1113,7 +1048,6 @@ static int elmcan_ldisc_open(struct tty_struct *tty)
        tty->receive_room = 65536; /* We don't flow control */
        elm->txleft = 0; /* Clear TTY TX buffer */
        spin_lock_init(&elm->lock);
-       atomic_set(&elm->refcount, 1);
        INIT_WORK(&elm->tx_work, elmcan_ldisc_tx_worker);
 
        /* Configure CAN metadata */
@@ -1156,26 +1090,13 @@ out_err:
  */
 static void elmcan_ldisc_close(struct tty_struct *tty)
 {
-       struct elmcan *elm = get_elm(tty);
-
-       if (!elm)
-               return;
+       struct elmcan *elm = (struct elmcan *)tty->disc_data;
 
        /* unregister_netdev() calls .ndo_stop() so we don't have to. */
        unregister_candev(elm->dev);
 
-       /* Decrease the refcount twice, once for our own get_elm(),
-        * and once to remove the count of 1 that we set in _open().
-        * Once it reaches 0, we can safely destroy it.
-        */
-       put_elm(elm);
-       put_elm(elm);
-
-       while (atomic_read(&elm->refcount) > 0)
-               msleep_interruptible(10);
-
-       /* At this point, all ldisc calls to us have become no-ops. */
-
+       /* Ensure that our worker won't be rescheduled */
+       clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
        flush_work(&elm->tx_work);
 
        /* Mark channel as dead */
@@ -1190,47 +1111,26 @@ static void elmcan_ldisc_close(struct tty_struct *tty)
        free_candev(elm->dev);
 }
 
-#if LINUX_VERSION_CODE < KERNEL_VERSION(5,16,0)
-static int elmcan_ldisc_hangup(struct tty_struct *tty)
-#else
-static void elmcan_ldisc_hangup(struct tty_struct *tty)
-#endif
-{
-       elmcan_ldisc_close(tty);
-#if LINUX_VERSION_CODE < KERNEL_VERSION(5,16,0)
-       return 0;
-#endif
-}
-
 static int elmcan_ldisc_ioctl(struct tty_struct *tty,
 #if LINUX_VERSION_CODE < KERNEL_VERSION(5,17,0)
                              struct file *file,
 #endif
                              unsigned int cmd, unsigned long arg)
 {
-       struct elmcan *elm = get_elm(tty);
+       struct elmcan *elm = (struct elmcan *)tty->disc_data;
        unsigned int tmp;
 
-       if (!elm)
-               return -EINVAL;
-
        switch (cmd) {
        case SIOCGIFNAME:
                tmp = strnlen(elm->dev->name, IFNAMSIZ - 1) + 1;
-               if (copy_to_user((void __user *)arg, elm->dev->name, tmp)) {
-                       put_elm(elm);
+               if (copy_to_user((void __user *)arg, elm->dev->name, tmp))
                        return -EFAULT;
-               }
-
-               put_elm(elm);
                return 0;
 
        case SIOCSIFHWADDR:
-               put_elm(elm);
                return -EINVAL;
 
        default:
-               put_elm(elm);
 #if LINUX_VERSION_CODE < KERNEL_VERSION(5,16,0)
                return tty_mode_ioctl(tty, file, cmd, arg);
 #else
@@ -1247,7 +1147,6 @@ static struct tty_ldisc_ops elmcan_ldisc = {
        .write_wakeup   = elmcan_ldisc_tx_wakeup,
        .open           = elmcan_ldisc_open,
        .close          = elmcan_ldisc_close,
-       .hangup         = elmcan_ldisc_hangup,
        .ioctl          = elmcan_ldisc_ioctl,
 };