Stop leaking SKBs in elm327_parse_frame()

[elmcan.git] / module / can327.c

diff --git a/module/can327.c b/module/can327.c
index 97ca7e148d334607dddbb7dfe02804d6e04c4374..ea92daf43bca731ce07d97cf46b2f114a0eaf3fb 100644 (file)
--- a/module/can327.c
+++ b/module/can327.c
@@ -14,14 +14,10 @@
 
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/moduleparam.h>
 
-#include <linux/atomic.h>
 #include <linux/bitops.h>
 #include <linux/ctype.h>
-#include <linux/delay.h>
 #include <linux/errno.h>
-#include <linux/if_ether.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/lockdep.h>
@@ -57,7 +53,7 @@
 
 #define ELM327_NAPI_WEIGHT 4
 
-#define ELM327_SIZE_RXBUF 224
+#define ELM327_SIZE_RXBUF 992
 #define ELM327_SIZE_TXBUF 32
 
 #define ELM327_CAN_CONFIG_SEND_SFF           0x8000
@@ -336,7 +332,7 @@ static inline void elm327_uart_side_failure(struct can327 *elm)
  * The reason to use strings is so we can easily include them in the C code,
  * and to avoid hardcoding lengths.
  */
-static inline int elm327_rxbuf_cmp(const u8 *buf, size_t nbytes, const char *reference)
+static inline bool elm327_rxbuf_cmp(const u8 *buf, size_t nbytes, const char *reference)
 {
        size_t ref_len = strlen(reference);
 
@@ -444,6 +440,7 @@ static int elm327_parse_frame(struct can327 *elm, size_t len)
                /* The line is likely garbled anyway, so bail.
                 * The main code will restart listening.
                 */
+               kfree_skb(skb);
                return -ENODATA;
        }
 
@@ -462,6 +459,7 @@ static int elm327_parse_frame(struct can327 *elm, size_t len)
                /* This is not a well-formatted data line.
                 * Assume it's an error message.
                 */
+               kfree_skb(skb);
                return -ENODATA;
        }
 
@@ -469,6 +467,7 @@ static int elm327_parse_frame(struct can327 *elm, size_t len)
                /* The line is too short to be a valid frame hex dump.
                 * Something interrupted the hex dump or it is invalid.
                 */
+               kfree_skb(skb);
                return -ENODATA;
        }
 
@@ -520,6 +519,7 @@ static int elm327_parse_frame(struct can327 *elm, size_t len)
                 * However, this will correctly drop the state machine back into
                 * command mode.
                 */
+               kfree_skb(skb);
                return -ENODATA;
        }
 
@@ -682,10 +682,9 @@ static void elm327_drop_bytes(struct can327 *elm, size_t i)
        elm->rxfill -= i;
 }
 
-static void elm327_parse_rxbuf(struct can327 *elm)
+static void elm327_parse_rxbuf(struct can327 *elm, size_t first_new_char_idx)
 {
-       size_t len;
-       int i;
+       size_t len, pos;
 
        lockdep_assert_held(&elm->lock);
 
@@ -696,20 +695,20 @@ static void elm327_parse_rxbuf(struct can327 *elm)
 
        case ELM327_STATE_GETDUMMYCHAR:
                /* Wait for 'y' or '>' */
-               for (i = 0; i < elm->rxfill; i++) {
-                       if (elm->rxbuf[i] == ELM327_DUMMY_CHAR) {
+               for (pos = 0; pos < elm->rxfill; pos++) {
+                       if (elm->rxbuf[pos] == ELM327_DUMMY_CHAR) {
                                elm327_send(elm, "\r", 1);
                                elm->state = ELM327_STATE_GETPROMPT;
-                               i++;
+                               pos++;
                                break;
-                       } else if (elm327_is_ready_char(elm->rxbuf[i])) {
+                       } else if (elm327_is_ready_char(elm->rxbuf[pos])) {
                                elm327_send(elm, ELM327_DUMMY_STRING, 1);
-                               i++;
+                               pos++;
                                break;
                        }
                }
 
-               elm327_drop_bytes(elm, i);
+               elm327_drop_bytes(elm, pos);
                break;
 
        case ELM327_STATE_GETPROMPT:
@@ -722,7 +721,7 @@ static void elm327_parse_rxbuf(struct can327 *elm)
 
        case ELM327_STATE_RECEIVING:
                /* Find <CR> delimiting feedback lines. */
-               len = 0;
+               len = first_new_char_idx;
                while (len < elm->rxfill && elm->rxbuf[len] != '\r')
                        len++;
 
@@ -756,13 +755,23 @@ static void elm327_parse_rxbuf(struct can327 *elm)
 
                        /* More data to parse? */
                        if (elm->rxfill)
-                               elm327_parse_rxbuf(elm);
+                               elm327_parse_rxbuf(elm, 0);
                }
        }
 }
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(5,10,0)
 /* Dummy needed to use can_rx_offload */
+#if LINUX_VERSION_CODE < KERNEL_VERSION(5,5,0)
+static unsigned int *can327_mailbox_read(struct can_rx_offload *offload,
+                                        struct can_frame *cf,
+                                        u32 *timestamp, unsigned int n)
+{
+       WARN_ON_ONCE(1); /* This function is a dummy, so don't call it! */
+
+       return -ENOBUFS;
+}
+#else /* Since 4e9c9484b085 (included in v5.5) */
 static struct sk_buff *can327_mailbox_read(struct can_rx_offload *offload,
                                           unsigned int n, u32 *timestamp,
                                           bool drop)
@@ -772,6 +781,7 @@ static struct sk_buff *can327_mailbox_read(struct can_rx_offload *offload,
        return ERR_PTR(-ENOBUFS);
 }
 #endif
+#endif
 
 static int can327_netdev_open(struct net_device *dev)
 {
@@ -896,20 +906,30 @@ static const struct net_device_ops can327_netdev_ops = {
        .ndo_change_mtu = can_change_mtu,
 };
 
-static bool can327_is_valid_rx_char(char c)
+static bool can327_is_valid_rx_char(u8 c)
 {
-       return (isdigit(c) ||
-               isupper(c) ||
-               c == ELM327_DUMMY_CHAR ||
-               c == ELM327_READY_CHAR ||
-               c == '<' ||
-               c == 'a' ||
-               c == 'b' ||
-               c == 'v' ||
-               c == '.' ||
-               c == '?' ||
-               c == '\r' ||
-               c == ' ');
+       static const bool lut_char_is_valid['z'] = {
+               ['\r'] = true,
+               [' '] = true,
+               ['.'] = true,
+               ['0'] = true, true, true, true, true,
+               ['5'] = true, true, true, true, true,
+               ['<'] = true,
+               [ELM327_READY_CHAR] = true,
+               ['?'] = true,
+               ['A'] = true, true, true, true, true, true, true,
+               ['H'] = true, true, true, true, true, true, true,
+               ['O'] = true, true, true, true, true, true, true,
+               ['V'] = true, true, true, true, true,
+               ['a'] = true,
+               ['b'] = true,
+               ['v'] = true,
+               [ELM327_DUMMY_CHAR] = true,
+       };
+       BUILD_BUG_ON(ELM327_DUMMY_CHAR >= 'z');
+
+       return (c < ARRAY_SIZE(lut_char_is_valid) &&
+               lut_char_is_valid[c]);
 }
 
 /* Handle incoming ELM327 ASCII data.
@@ -925,12 +945,18 @@ static void can327_ldisc_rx(struct tty_struct *tty,
 #endif
 {
        struct can327 *elm = (struct can327 *)tty->disc_data;
+       size_t first_new_char_idx;
 
        if (elm->uart_side_failure)
                return;
 
        spin_lock_bh(&elm->lock);
 
+       /* Store old rxfill, so elm327_parse_rxbuf() will have
+        * the option of skipping already checked characters.
+        */
+       first_new_char_idx = elm->rxfill;
+
        while (count-- && elm->rxfill < ELM327_SIZE_RXBUF) {
                if (fp && *fp++) {
                        netdev_err(elm->dev, "Error in received character stream. Check your wiring.");
@@ -967,7 +993,7 @@ static void can327_ldisc_rx(struct tty_struct *tty,
        }
 
        if (count >= 0) {
-               netdev_err(elm->dev, "Receive buffer overflowed. Bad chip or wiring?");
+               netdev_err(elm->dev, "Receive buffer overflowed. Bad chip or wiring? count = %i", count);
 
                elm327_uart_side_failure(elm);
 
@@ -975,7 +1001,7 @@ static void can327_ldisc_rx(struct tty_struct *tty,
                return;
        }
 
-       elm327_parse_rxbuf(elm);
+       elm327_parse_rxbuf(elm, first_new_char_idx);
        spin_unlock_bh(&elm->lock);
 }