| 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
| ||
|
Message-ID: <YmK83NfVqEvGg8DW@kroah.com>
Date: Fri, 22 Apr 2022 16:34:04 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: linux-serial@...r.kernel.org, Jiri Slaby <jirislaby@...nel.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
linux-kernel@...r.kernel.org,
Gilles Buloz <gilles.buloz@...tron.com>,
Johan Hovold <johan@...nel.org>
Subject: Re: [PATCH v3 3/5] tty: Add lookahead param to receive_buf
On Mon, Apr 11, 2022 at 12:48:57PM +0300, Ilpo Järvinen wrote:
> After lookahead for XON/XOFF characters is added by the next
> patch,
There is no "next" in a series. This commit needs to be justified here,
right?
> the receive side needs to ensure the flow-control
> actions are not retaken later on when those same characters
> get received by TTY.
>
> Thus, pass lookahead count to receive_buf and skip
> flow-control character actions if already taken for the
> character in question. Lookahead count will become live after
> the next patch.
You have all 72 columns, please use them.
>
> The logic in n_tty_receive_char_closing is bit tricky.
> The lookahead_done checks cannot be combined with the main
> conditions because doing so would give chance for the part
> after || in the else if condition to execute when lookahead
> done is set (rather than just skipping the flow-control
> character like it should).
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> ---
> drivers/accessibility/speakup/spk_ttyio.c | 2 +-
> drivers/bluetooth/hci_ldisc.c | 2 +-
> drivers/char/pcmcia/synclink_cs.c | 2 +-
> drivers/input/serio/serport.c | 4 +-
> drivers/isdn/capi/capi.c | 2 +-
> drivers/misc/ti-st/st_core.c | 2 +-
> drivers/net/caif/caif_serial.c | 4 +-
> drivers/net/can/slcan.c | 2 +-
> drivers/net/hamradio/6pack.c | 4 +-
> drivers/net/hamradio/mkiss.c | 2 +-
> drivers/net/mctp/mctp-serial.c | 2 +-
> drivers/net/ppp/ppp_async.c | 2 +-
> drivers/net/ppp/ppp_synctty.c | 2 +-
> drivers/net/slip/slip.c | 2 +-
> drivers/tty/n_gsm.c | 2 +-
> drivers/tty/n_hdlc.c | 2 +-
> drivers/tty/n_null.c | 2 +-
> drivers/tty/n_tty.c | 53 ++++++++++++++---------
> drivers/tty/serdev/serdev-ttyport.c | 3 +-
> drivers/tty/synclink_gt.c | 2 +-
> drivers/tty/tty_buffer.c | 8 ++--
> drivers/tty/tty_io.c | 2 +-
> drivers/tty/tty_port.c | 5 ++-
> drivers/tty/vt/selection.c | 2 +-
> include/linux/tty_flip.h | 2 +-
> include/linux/tty_ldisc.h | 20 ++++++---
> include/linux/tty_port.h | 3 +-
> net/nfc/nci/uart.c | 2 +-
> sound/soc/codecs/cx20442.c | 2 +-
> sound/soc/ti/ams-delta.c | 2 +-
> 30 files changed, 83 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/accessibility/speakup/spk_ttyio.c b/drivers/accessibility/speakup/spk_ttyio.c
> index 08cf8a17754b..b33536eea1d3 100644
> --- a/drivers/accessibility/speakup/spk_ttyio.c
> +++ b/drivers/accessibility/speakup/spk_ttyio.c
> @@ -73,7 +73,7 @@ static void spk_ttyio_ldisc_close(struct tty_struct *tty)
>
> static int spk_ttyio_receive_buf2(struct tty_struct *tty,
> const unsigned char *cp,
> - const char *fp, int count)
> + const char *fp, int count, unsigned int lookahead_count)
Ick, adding yet-another-parameter to a function is a mess as it's hard
to know what to do with this and what it means just by looking at when
it is called.
> {
> struct spk_ldisc_data *ldisc_data = tty->disc_data;
> struct spk_synth *synth = ldisc_data->synth;
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index f537673ede17..08c329a4cdcc 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -596,7 +596,7 @@ static void hci_uart_tty_wakeup(struct tty_struct *tty)
> * Return Value: None
> */
> static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
> - const char *flags, int count)
> + const char *flags, int count, unsigned int lookahead_count)
> {
> struct hci_uart *hu = tty->disc_data;
>
> diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
> index 78baba55a8b5..de9c151cfb12 100644
> --- a/drivers/char/pcmcia/synclink_cs.c
> +++ b/drivers/char/pcmcia/synclink_cs.c
> @@ -501,7 +501,7 @@ static void ldisc_receive_buf(struct tty_struct *tty,
> ld = tty_ldisc_ref(tty);
> if (ld) {
> if (ld->ops->receive_buf)
> - ld->ops->receive_buf(tty, data, flags, count);
> + ld->ops->receive_buf(tty, data, flags, count, 0);
> tty_ldisc_deref(ld);
> }
> }
> diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
> index 669a728095b8..f0f19b5a2059 100644
> --- a/drivers/input/serio/serport.c
> +++ b/drivers/input/serio/serport.c
> @@ -114,8 +114,8 @@ static void serport_ldisc_close(struct tty_struct *tty)
> * 'interrupt' routine.
> */
>
> -static void serport_ldisc_receive(struct tty_struct *tty,
> - const unsigned char *cp, const char *fp, int count)
> +static void serport_ldisc_receive(struct tty_struct *tty, const unsigned char *cp,
> + const char *fp, int count, unsigned int lookahead_count)
> {
> struct serport *serport = (struct serport*) tty->disc_data;
> unsigned long flags;
> diff --git a/drivers/isdn/capi/capi.c b/drivers/isdn/capi/capi.c
> index 0f00be62438d..beb4c78a7219 100644
> --- a/drivers/isdn/capi/capi.c
> +++ b/drivers/isdn/capi/capi.c
> @@ -454,7 +454,7 @@ static int handle_recv_skb(struct capiminor *mp, struct sk_buff *skb)
> skb_pull(skb, CAPIMSG_LEN(skb->data));
> pr_debug("capi: DATA_B3_RESP %u len=%d => ldisc\n",
> datahandle, skb->len);
> - ld->ops->receive_buf(tty, skb->data, NULL, skb->len);
> + ld->ops->receive_buf(tty, skb->data, NULL, skb->len, 0);
> } else {
> printk(KERN_ERR "capi: send DATA_B3_RESP failed=%x\n",
> errcode);
> diff --git a/drivers/misc/ti-st/st_core.c b/drivers/misc/ti-st/st_core.c
> index 7f6976a9f508..b2c96d72c0e3 100644
> --- a/drivers/misc/ti-st/st_core.c
> +++ b/drivers/misc/ti-st/st_core.c
> @@ -797,7 +797,7 @@ static void st_tty_close(struct tty_struct *tty)
> }
>
> static void st_tty_receive(struct tty_struct *tty, const unsigned char *data,
> - const char *tty_flags, int count)
> + const char *tty_flags, int count, unsigned int lookahead_count)
> {
> #ifdef VERBOSE
> print_hex_dump(KERN_DEBUG, ">in>", DUMP_PREFIX_NONE,
> diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
> index 688075859ae4..cc97c436ec88 100644
> --- a/drivers/net/caif/caif_serial.c
> +++ b/drivers/net/caif/caif_serial.c
> @@ -159,7 +159,7 @@ static inline void debugfs_tx(struct ser_device *ser, const u8 *data, int size)
> #endif
>
> static void ldisc_receive(struct tty_struct *tty, const u8 *data,
> - const char *flags, int count)
> + const char *flags, int count, unsigned int lookahead_count)
> {
> struct sk_buff *skb = NULL;
> struct ser_device *ser;
> @@ -237,7 +237,7 @@ static int handle_tx(struct ser_device *ser)
> update_tty_status(ser);
> } else {
> tty_wr = len;
> - ldisc_receive(tty, skb->data, NULL, len);
> + ldisc_receive(tty, skb->data, NULL, len, 0);
See, what does 0 here mean? Who knows? Not a good api :(
> }
> ser->dev->stats.tx_packets++;
> ser->dev->stats.tx_bytes += tty_wr;
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index ec294d0c5722..5e03e14c2d99 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -471,7 +471,7 @@ static void slc_setup(struct net_device *dev)
>
> static void slcan_receive_buf(struct tty_struct *tty,
> const unsigned char *cp, const char *fp,
> - int count)
> + int count, unsigned int lookahead_count)
> {
> struct slcan *sl = (struct slcan *) tty->disc_data;
>
> diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
> index 45c3c4a1101b..6ba4b05dff78 100644
> --- a/drivers/net/hamradio/6pack.c
> +++ b/drivers/net/hamradio/6pack.c
> @@ -426,8 +426,8 @@ static void sixpack_write_wakeup(struct tty_struct *tty)
> * a block of 6pack data has been received, which can now be decapsulated
> * and sent on to some IP layer for further processing.
> */
> -static void sixpack_receive_buf(struct tty_struct *tty,
> - const unsigned char *cp, const char *fp, int count)
> +static void sixpack_receive_buf(struct tty_struct *tty, const unsigned char *cp,
> + const char *fp, int count, unsigned int lookahead_count)
> {
> struct sixpack *sp;
> int count1;
> diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
> index c251e04ae047..498997ef429a 100644
> --- a/drivers/net/hamradio/mkiss.c
> +++ b/drivers/net/hamradio/mkiss.c
> @@ -875,7 +875,7 @@ static int mkiss_ioctl(struct tty_struct *tty, unsigned int cmd,
> * and sent on to the AX.25 layer for further processing.
> */
> static void mkiss_receive_buf(struct tty_struct *tty, const unsigned char *cp,
> - const char *fp, int count)
> + const char *fp, int count, unsigned int lookahead_count)
> {
> struct mkiss *ax = mkiss_get(tty);
>
> diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
> index 7cd103fd34ef..f1dfab4c54cb 100644
> --- a/drivers/net/mctp/mctp-serial.c
> +++ b/drivers/net/mctp/mctp-serial.c
> @@ -390,7 +390,7 @@ static void mctp_serial_push(struct mctp_serial *dev, unsigned char c)
>
> static void mctp_serial_tty_receive_buf(struct tty_struct *tty,
> const unsigned char *c,
> - const char *f, int len)
> + const char *f, int len, unsigned int lookahead_count)
> {
> struct mctp_serial *dev = tty->disc_data;
> int i;
> diff --git a/drivers/net/ppp/ppp_async.c b/drivers/net/ppp/ppp_async.c
> index 15a179631903..87c205a02984 100644
> --- a/drivers/net/ppp/ppp_async.c
> +++ b/drivers/net/ppp/ppp_async.c
> @@ -338,7 +338,7 @@ ppp_asynctty_poll(struct tty_struct *tty, struct file *file, poll_table *wait)
> /* May sleep, don't call from interrupt level or with interrupts disabled */
> static void
> ppp_asynctty_receive(struct tty_struct *tty, const unsigned char *buf,
> - const char *cflags, int count)
> + const char *cflags, int count, unsigned int lookahead_count)
> {
> struct asyncppp *ap = ap_get(tty);
> unsigned long flags;
> diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
> index 18283b7b94bc..b82f4bd82b21 100644
> --- a/drivers/net/ppp/ppp_synctty.c
> +++ b/drivers/net/ppp/ppp_synctty.c
> @@ -331,7 +331,7 @@ ppp_sync_poll(struct tty_struct *tty, struct file *file, poll_table *wait)
> /* May sleep, don't call from interrupt level or with interrupts disabled */
> static void
> ppp_sync_receive(struct tty_struct *tty, const unsigned char *buf,
> - const char *cflags, int count)
> + const char *cflags, int count, unsigned int lookahead_count)
> {
> struct syncppp *ap = sp_get(tty);
> unsigned long flags;
> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> index 88396ff99f03..7fccf28256a8 100644
> --- a/drivers/net/slip/slip.c
> +++ b/drivers/net/slip/slip.c
> @@ -686,7 +686,7 @@ static void sl_setup(struct net_device *dev)
> */
>
> static void slip_receive_buf(struct tty_struct *tty, const unsigned char *cp,
> - const char *fp, int count)
> + const char *fp, int count, unsigned int lookahead_count)
> {
> struct slip *sl = tty->disc_data;
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index fa92f727fdf8..45f022892b28 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2500,7 +2500,7 @@ static void gsmld_detach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
> }
>
> static void gsmld_receive_buf(struct tty_struct *tty, const unsigned char *cp,
> - const char *fp, int count)
> + const char *fp, int count, unsigned int lookahead_count)
> {
> struct gsm_mux *gsm = tty->disc_data;
> char flags = TTY_NORMAL;
> diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
> index 94c1ec2dd754..2d7c94a38a92 100644
> --- a/drivers/tty/n_hdlc.c
> +++ b/drivers/tty/n_hdlc.c
> @@ -379,7 +379,7 @@ static void n_hdlc_tty_wakeup(struct tty_struct *tty)
> * interpreted as one HDLC frame.
> */
> static void n_hdlc_tty_receive(struct tty_struct *tty, const __u8 *data,
> - const char *flags, int count)
> + const char *flags, int count, unsigned int lookahead_count)
> {
> register struct n_hdlc *n_hdlc = tty->disc_data;
> register struct n_hdlc_buf *buf;
> diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c
> index f913b665af72..6bce76b5bb1c 100644
> --- a/drivers/tty/n_null.c
> +++ b/drivers/tty/n_null.c
> @@ -34,7 +34,7 @@ static ssize_t n_null_write(struct tty_struct *tty, struct file *file,
>
> static void n_null_receivebuf(struct tty_struct *tty,
> const unsigned char *cp, const char *fp,
> - int cnt)
> + int cnt, unsigned int lookahead_count)
> {
> }
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 90b3e06cbeb1..708cc85ac43d 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -1226,11 +1226,15 @@ static bool n_tty_is_char_flow_ctrl(struct tty_struct *tty, unsigned char c)
> }
>
> /* Returns true if c is consumed as flow-control character */
> -static bool n_tty_receive_char_flow_ctrl(struct tty_struct *tty, unsigned char c)
> +static bool n_tty_receive_char_flow_ctrl(struct tty_struct *tty, unsigned char c,
> + bool lookahead_done)
> {
> if (!n_tty_is_char_flow_ctrl(tty, c))
> return false;
>
> + if (lookahead_done)
> + return true;
Why would this function be called if this option was true?
> +
> if (c == START_CHAR(tty)) {
> start_tty(tty);
> process_echoes(tty);
> @@ -1241,11 +1245,12 @@ static bool n_tty_receive_char_flow_ctrl(struct tty_struct *tty, unsigned char c
> return true;
> }
>
> -static void n_tty_receive_char_special(struct tty_struct *tty, unsigned char c)
> +static void n_tty_receive_char_special(struct tty_struct *tty, unsigned char c,
> + bool lookahead_done)
> {
> struct n_tty_data *ldata = tty->disc_data;
>
> - if (I_IXON(tty) && n_tty_receive_char_flow_ctrl(tty, c))
> + if (I_IXON(tty) && n_tty_receive_char_flow_ctrl(tty, c, lookahead_done))
> return;
>
> if (L_ISIG(tty)) {
> @@ -1400,7 +1405,8 @@ static void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
> put_tty_queue(c, ldata);
> }
>
> -static void n_tty_receive_char_closing(struct tty_struct *tty, unsigned char c)
> +static void n_tty_receive_char_closing(struct tty_struct *tty, unsigned char c,
> + bool lookahead_done)
bool here, yet characters elsewhere? That's messy :(
the overall idea is good, this implementation isn't quite there yet.
I did take the first patch in the series, it made sense.
thanks,
greg k-h
Powered by blists - more mailing lists