drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
Subsystems (2)
Hello Greg Kroah-Hartman:
for MAX_ASYNC_BUFFER_SIZE:
it is defined as 4096;
but for the max buffer size which it processes, is 65535.
so suggest to #define MAX_ASYNC_BUFFER_SIZE 0x10000 (better than 0xffff)
I use 3 Step to prove it, please see below:
by the way:
I find it only through code review, not test it.
so I send it as suggestions (not a patch).
next time:
for the same case, if it is better to send a patch, directly. please tell me by replying for this mail.
(at least, it can save your time, since you are busy)
gchen.
---------------------------------------------------------------------------------
Step 1:
the MAX_ASYNC_BUFFER_SIZE is 4096:
root@gchen-desktop:~/linux# grep -rn MAX_ASYNC_BUFFER_SIZE * drivers/char/pcmcia/synclink_cs.c:213: char flag_buf[MAX_ASYNC_BUFFER_SIZE]; drivers/tty/synclink_gt.c:320: char flag_buf[MAX_ASYNC_BUFFER_SIZE]; drivers/tty/synclink_gt.c:321: char char_buf[MAX_ASYNC_BUFFER_SIZE]; drivers/tty/synclink.c:294: char flag_buf[MAX_ASYNC_BUFFER_SIZE]; drivers/tty/synclink.c:295: char char_buf[MAX_ASYNC_BUFFER_SIZE]; drivers/tty/synclinkmp.c:265: char flag_buf[MAX_ASYNC_BUFFER_SIZE]; drivers/tty/synclinkmp.c:266: char char_buf[MAX_ASYNC_BUFFER_SIZE]; include/uapi/linux/synclink.h:54:#define MAX_ASYNC_BUFFER_SIZE 4096
---------------------------------------------------------------------------------
Step 2:
one sample in drivers/char/pcmcia/synclink_cs.c: (same for another files)
we check the framesize according to info->max_frame_size in line 3687..3689.
and call ldisc_receive_buf without checking whether it is larger than MAX_ASYNC_BUFFER_SIZE at line 3705
info->max_frame_size can be the value between 4096 and 65535 in line 2729..2732. ldisc_receive_buf call ld->ops->receive_buf to perform the work.
496 static void ldisc_receive_buf(struct tty_struct *tty,
497 const __u8 *data, char *flags, int count)
498 {
499 struct tty_ldisc *ld;
500 if (!tty)
501 return;
502 ld = tty_ldisc_ref(tty);
503 if (ld) {
504 if (ld->ops->receive_buf)
505 ld->ops->receive_buf(tty, data, flags, count);
506 tty_ldisc_deref(ld);
507 }
508 }
...
2728
2729 if (info->max_frame_size < 4096)
2730 info->max_frame_size = 4096;
2731 else if (info->max_frame_size > 65535)
2732 info->max_frame_size = 65535;
2733
...
3686 if (framesize) {
3687 if ((info->params.crc_type & HDLC_CRC_RETURN_EX &&
3688 framesize+1 > info->max_frame_size) ||
3689 framesize > info->max_frame_size)
3690 info->icount.rxlong++;
3691 else {
3692 if (status & BIT5)
3693 info->icount.rxok++;
3694
3695 if (info->params.crc_type & HDLC_CRC_RETURN_EX) {
3696 *(buf->data + framesize) = status & BIT5 ? RX_OK:RX_CRC_ERROR;
3697 ++framesize;
3698 }
3699
3700 #if SYNCLINK_GENERIC_HDLC
3701 if (info->netcount)
3702 hdlcdev_rx(info, buf->data, framesize);
3703 else
3704 #endif
3705 ldisc_receive_buf(tty, buf->data, info->flag_buf, framesize);
3706 }
3707 }
---------------------------------------------------------------------------------
Step 3:
one sample in drivers/tty/n_gsm.c (same for another implementation)
receive_buf is a function ptr which may be gsmld_receive_buf at line 2819. it does not check the length of count whether larger than MAX_ASYNC_BUFFER_SIZE. if count is larger than MAX_ASYNC_BUFFER_SIZE, will cause issue.
it is only a sample, maybe not the function ptr which mentioned in Step 2.
at lease, I have checked 3 function ptr implementations, none of them check the MAX_ASYNC_BUFFER_SIZE. the all function ptr searching is at the bottom.
2257 static void gsmld_receive_buf(struct tty_struct *tty, const unsigned char *cp,
2258 char *fp, int count)
2259 {
2260 struct gsm_mux *gsm = tty->disc_data; 2261 const unsigned char *dp; 2262 char *f; 2263 int i; 2264 char buf[64]; 2265 char flags;
2266
2267 if (debug & 4)
2268 print_hex_dump_bytes("gsmld_receive: ", DUMP_PREFIX_OFFSET,
2269 cp, count);
2270
2271 for (i = count, dp = cp, f = fp; i; i--, dp++) {
2272 flags = *f++;
2273 switch (flags) {
2274 case TTY_NORMAL:
2275 gsm->receive(gsm, *dp);
2276 break;
2277 case TTY_OVERRUN:
2278 case TTY_BREAK:
2279 case TTY_PARITY:
2280 case TTY_FRAME:2281 gsm->error(gsm, *dp, flags);
2282 break;
2283 default:
2284 WARN_ONCE(1, "%s: unknown flag %d\n",
2285 tty_name(tty, buf), flags);
2286 break;
2287 }
2288 } 2289 /* FASYNC if needed ? */ 2290 /* If clogged call tty_throttle(tty); */
2291 }
2292
...
2806 /* Line discipline for real tty */
2807 struct tty_ldisc_ops tty_ldisc_packet = {
2808 .owner = THIS_MODULE, 2809 .magic = TTY_LDISC_MAGIC, 2810 .name = "n_gsm", 2811 .open = gsmld_open, 2812 .close = gsmld_close, 2813 .flush_buffer = gsmld_flush_buffer, 2814 .chars_in_buffer = gsmld_chars_in_buffer, 2815 .read = gsmld_read, 2816 .write = gsmld_write, 2817 .ioctl = gsmld_ioctl, 2818 .poll = gsmld_poll, 2819 .receive_buf = gsmld_receive_buf, 2820 .write_wakeup = gsmld_write_wakeup
2821 };
2822
input/serio/serport.c:244: .receive_buf = serport_ldisc_receive,isdn/gigaset/ser-gigaset.c:758: .receive_buf = gigaset_tty_receive,misc/ti-st/st_core.c:822: .receive_buf = st_tty_receive,net/can/slcan.c:626: .receive_buf = slcan_receive_buf,net/hamradio/6pack.c:808: .receive_buf = sixpack_receive_buf,net/hamradio/mkiss.c:996: .receive_buf = mkiss_receive_buf,net/irda/irtty-sir.c:547: .receive_buf = irtty_receive_buf,net/caif/caif_serial.c:378: .receive_buf = ldisc_receive,net/ppp/ppp_synctty.c:428: .receive_buf = ppp_sync_receive,net/ppp/ppp_async.c:387: .receive_buf = ppp_asynctty_receive,tty/n_gsm.c:2819: .receive_buf = gsmld_receive_buf,tty/n_tty.c:2129: .receive_buf = n_tty_receive_buf,tty/n_hdlc.c:234: .receive_buf = n_hdlc_tty_receive,tty/n_tracerouter.c:193: .receive_buf = n_tracerouter_receivebuftty/n_r3964.c:156: .receive_buf = r3964_receive_buf,
Greg KH
maintainerRe: 【Suggestion】drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
In reply to a message not in the archiveQuoted 6 lines · Chen Gang
Hello Greg Kroah-Hartman:
for
MAX_ASYNC_BUFFER_SIZE:
it is defined as 4096;
but for the max buffer size which it processes, is 65535.
so suggest to #defineMAX_ASYNC_BUFFER_SIZE0x10000(better than0xffff)
Please, send tty questions to the [email protected] list also.
And, I really don't understand here, why do you want to change this? What is it going to change? And why?
greg k-h
Quoted 10 lines · Greg KH
Quoted 6 lines · Chen Gang
Hello Greg Kroah-Hartman:
for
MAX_ASYNC_BUFFER_SIZE:
it is defined as 4096;
but for the max buffer size which it processes, is 65535.
so suggest to #defineMAX_ASYNC_BUFFER_SIZE0x10000(better than0xffff)Please, send tty questions to the [email protected] list also.
I cc to [email protected], in this reply.
I referenced the file MAINTAINERS, before sent original mail:
it seems all drivers/tty/serial/* are relative with
[email protected]. but our case is not relative drivers/tty/serial. so for not bother them, I am not send to them, originally.
in MAINTAINERS, line 7438, I find for common of driver/tty/*, can send
to you and no cc, so I send you and cc to [email protected].
next time, for all tty questions, I will cc to
[email protected] (not cc to [email protected]).
Quoted 4 lines
And, I really don't understand here, why do you want to change this? What is it going to change? And why?
Why:
for the context MGSLPC_INFO *info in drivers/char/pcmcia/synclink_cs.c
info->max_frame_size can be the value between 4096 .. 65535 (can be
set by its module input parameter)
info->flag_buf length is 4096 (MAX_ASYNC_BUFFER_SIZE)
in function rx_get_frame
the framesize is limit by info->max_frame_size, but may still be
larger that 4096.
when call function ldisc_receive_buf, info->flag_buf is equal to4096, but framesize can be more than 4096. it will cause memory over flow.
What:
#define MAX_ASYNC_BUFFER_SIZE 0x10000 (instead of 4096, originally). let it match the max frame size.
At last:
my suggestion may be incorrect, need relative member (who expert about
it) to help checking.
welcome another suggestion or completions.
thanks.
gchen.
Quoted 8 lines
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 3 lines
Chen Gang Asianux Corporation
PATCH 1/1 MAINTAINERS: TTY - Add linux-serial mailing list
--- MAINTAINERS | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
MAINTAINERS +1 -0
@@ -7658,6 +7658,7 @@ K: ^Subject:.*(?i)trivial TTY LAYER M: Greg Kroah-Hartman <[email protected]> M: Jiri Slaby <[email protected]> +L: [email protected] S: Supported T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git F: drivers/tty/
Re: [PATCH] MAINTAINERS: TTY - Add linux-serial mailing list
Quoted 14 lines · Joe Perches
Signed-off-by Joe Perches <[email protected]>--- MAINTAINERS | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)MAINTAINERS +0
-0
MAINTAINERS K: ^Subject:.*(?i)trivial · line 7664
Quoted 7 lines
MAINTAINERS +1
-0@@ -7658,6 +7658,7 @@ K: ^Subject:.*(?i)trivial TTY LAYER M: Greg Kroah-Hartman <[email protected]> M: Jiri Slaby <[email protected]> +L: [email protected] S: Supported T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git F: drivers/tty/
thank you very much.
Signature · 7 lines
Chen Gang Asianux Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] MAINTAINERS: TTY - Add linux-serial mailing list
In reply to Joe PerchesQuoted 10 lines · Joe Perches
Signed-off-by Joe Perches <[email protected]>--- MAINTAINERS | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)MAINTAINERS +0
-0
MAINTAINERS K: ^Subject:.*(?i)trivial · line 7661
Quoted 4 lines
MAINTAINERS +1
-0@@ -7658,6 +7658,7 @@ K: ^Subject:.*(?i)trivial TTY LAYER M: Greg Kroah-Hartman <[email protected]> M: Jiri Slaby <[email protected]> +L: [email protected]
This might have the effect that people will try to reach us at that list. But this is not true at least for me. serial != tty.
thanks,
Signature · 2 lines
js suse labs
Quoted 6 lines · On Thu, 29 Nov 2012 13:07:28 +0800 Chen Gang <[email protected]> wrote:
Hello Greg Kroah-Hartman:
for
MAX_ASYNC_BUFFER_SIZE:
it is defined as 4096;
but for the max buffer size which it processes, is 65535.
so suggest to #defineMAX_ASYNC_BUFFER_SIZE0x10000(better than0xffff)
I don't see the need to change this. Possibly some of the old synclink drivers need to check more carefully for overflows if configured for very large frame sizes ?
Quoted 9 lines
---------------------------------------------------------------------------------
Step 3:one sample in
drivers/tty/n_gsm.c(same for another implementation)
receive_bufis a function ptr which may begsmld_receive_bufat line 2819. it does not check the length of count whether larger thanMAX_ASYNC_BUFFER_SIZE. if count is larger thanMAX_ASYNC_BUFFER_SIZE, will cause issue.
Why should it - MAX_ASYNC_BUFFER_SIZE is an internal detail of the synclink drivers.
Alan
Greg KH
maintainerQuoted 6 lines · Chen Gang
Quoted 3 lines
And, I really don't understand here, why do you want to change this? What is it going to change? And why?
Why:
for the contextMGSLPC_INFO*info indrivers/char/pcmcia/synclink_cs.c
Quoted 8 lines
info->max_frame_size can be the value between 4096 .. 65535 (can be set by its module input parameter) info->flag_buf length is 4096 (MAX_ASYNC_BUFFER_SIZE) in function rx_get_frame the framesize is limit by info->max_frame_size, but may still be larger that 4096. when call function ldisc_receive_buf, info->flag_buf is equal to4096, but framesize can be more than 4096. it will cause memory over flow.
Do you use that pcmcia driver for anything? Are those cards still around?
Quoted 7 lines
What:
#defineMAX_ASYNC_BUFFER_SIZE0x10000(instead of 4096, originally). let it match the max frame size.At last:
my suggestion may be incorrect, need relative member (who expert about
it) to help checking.
That driver might be incorrect, yes, care to make up a patch for it and test it to verify it fixes the problem?
thanks,
greg k-h
Quoted 5 lines · Alan Cox
Quoted 2 lines · On Thu, 29 Nov 2012 13:07:28 +0800 Chen Gang <[email protected]> wrote:
Hello Greg Kroah-Hartman:
Quoted 9 lines
Quoted 4 lines
for
MAX_ASYNC_BUFFER_SIZE:
it is defined as 4096;
but for the max buffer size which it processes, is 65535.
so suggest to #defineMAX_ASYNC_BUFFER_SIZE0x10000(better than0xffff)I don't see the need to change this. Possibly some of the old synclink drivers need to check more carefully for overflows if configured for very large frame sizes ?
I am just through code review (so it is only a suggestion), I will try to perform test. also welcome another members to help testing.
this issue has effect with 4 synclink drivers (most of source code are the same).
drivers/char/pcmcia/synclink_cs.c:213: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink_gt.c:320: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink.c:294: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclinkmp.c:265: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
for the char_buf, has already useless (can be removed)
drivers/tty/synclink_gt.c:321: char char_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink.c:295: char char_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclinkmp.c:266: char char_buf[MAX_ASYNC_BUFFER_SIZE];
Quoted 6 lines
Quoted 6 lines
---------------------------------------------------------------------------------
Step 3:one sample in
drivers/tty/n_gsm.c(same for another implementation)
Quoted 10 lines
Quoted 3 lines
receive_bufis a function ptr which may begsmld_receive_bufat line 2819. it does not check the length of count whether larger thanMAX_ASYNC_BUFFER_SIZE. if count is larger thanMAX_ASYNC_BUFFER_SIZE, will cause issue.Why should it -
MAX_ASYNC_BUFFER_SIZEis an internal detail of the synclink drivers.Alan
no, not need. (excuse me, my English is not quite well, maybe you misunderstand what I said)
at least, currently:
the caller should be sure that the buffer length is enough (it seems not, I need test it). the internal has no duty to check it.
Signature · 3 lines
Chen Gang Asianux Corporation
Quoted 16 lines · Greg KH
Quoted 6 lines · Chen Gang
Quoted 3 lines
And, I really don't understand here, why do you want to change this? What is it going to change? And why?
Why:
for the contextMGSLPC_INFO*info indrivers/char/pcmcia/synclink_cs.cQuoted 8 lines
info->max_frame_size can be the value between 4096 .. 65535 (can be set by its module input parameter) info->flag_buf length is 4096 (MAX_ASYNC_BUFFER_SIZE) in function rx_get_frame the framesize is limit by info->max_frame_size, but may still be larger that 4096. when call function ldisc_receive_buf, info->flag_buf is equal to4096, but framesize can be more than 4096. it will cause memory over flow.
Quoted 2 lines
Do you use that pcmcia driver for anything? Are those cards still around?
I am not use them.
I am just through code review (so it is only a suggestion).
this issue has effect with 4 synclink drivers
I checked their source code, all of them have the same issue.
drivers/char/pcmcia/synclink_cs.c:213: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink_gt.c:320: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink.c:294: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclinkmp.c:265: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
by the way, for the char_buf, has already useless (can be removed)
drivers/tty/synclink_gt.c:321: char char_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink.c:295: char char_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclinkmp.c:266: char char_buf[MAX_ASYNC_BUFFER_SIZE];
Quoted 5 lines
Quoted 5 lines
What:
#defineMAX_ASYNC_BUFFER_SIZE0x10000(instead of 4096, originally). let it match the max frame size.
Quoted 7 lines
Quoted 3 lines
At last:
my suggestion may be incorrect, need relative member (who expert about
it) to help checking.That driver might be incorrect, yes, care to make up a patch for it and test it to verify it fixes the problem?
and now Alan Cox has his own opinions
at least, I think it is valuable to continue discussing about it.
if Alan Cox agree with it (but it seems not), I will make patch, and try to perform test. also welcome another members to help testing.
Quoted 5 lines
thanks,
greg k-h
Signature · 7 lines
Chen Gang Asianux Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · Chen Gang
Quoted 10 lines · Alan Cox
Quoted 6 lines · On Thu, 29 Nov 2012 13:07:28 +0800 Chen Gang <[email protected]> wrote:
Hello Greg Kroah-Hartman:
for
MAX_ASYNC_BUFFER_SIZE:
it is defined as 4096;
but for the max buffer size which it processes, is 65535.
so suggest to #defineMAX_ASYNC_BUFFER_SIZE0x10000(better than0xffff)
Quoted 5 lines
Quoted 4 lines
I don't see the need to change this. Possibly some of the old synclink drivers need to check more carefully for overflows if configured for very large frame sizes ?
sorry forget to reply "I don't see the need to change this"
I think what Alan Cox said is:
if it was necessary (surely overflows by testing):
not touch MAX_ASYNC_BUFFER_SIZE,
can judge the buffer whether larger than MAX_ASYNC_BUFFER_SIZE.
if larger, we can skip it.
I think we also have another 4 ways: (if surely overflows by testing)
I prefer:
use flag_buf[HDLC_MAX_FRAME_SIZE] instead of flag_buf[MAX_ASYNC_BUFFER_SIZE] it is the simplest and clearest way.
it will consume a little more memory, but it seems minor negative effect with global.
2nd way:
dynamically allocate relative buffer to fit the current max frame size (4096..65535). it is not complex, but can save a little memory
3rd way:
we have to make a loop to receive one frame.
it will be complex, need reconstruction current source code (and more testing).
4th way:
#define MAX_ASYNC_BUFFER_SIZE 0x10000
it is my original suggestion, but it seems not quite suitable.
welcome to giving your choice (or provide your new choice), thanks.
thanks.
gchen.
Quoted 39 lines
I am just through code review (so it is only a suggestion), I will try to perform test. also welcome another members to help testing.
this issue has effect with 4 synclink drivers (most of source code are the same).
drivers/char/pcmcia/synclink_cs.c:213: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink_gt.c:320: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink.c:294: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclinkmp.c:265: char flag_buf[MAX_ASYNC_BUFFER_SIZE];for the
char_buf, has already useless (can be removed)
drivers/tty/synclink_gt.c:321: char char_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink.c:295: char char_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclinkmp.c:266: char char_buf[MAX_ASYNC_BUFFER_SIZE];Quoted 6 lines
Quoted 6 lines
---------------------------------------------------------------------------------
Step 3:one sample in
drivers/tty/n_gsm.c(same for another implementation)Quoted 10 lines
Quoted 3 lines
receive_bufis a function ptr which may begsmld_receive_bufat line 2819. it does not check the length of count whether larger thanMAX_ASYNC_BUFFER_SIZE. if count is larger thanMAX_ASYNC_BUFFER_SIZE, will cause issue.Why should it -
MAX_ASYNC_BUFFER_SIZEis an internal detail of the synclink drivers.Alan
no, not need. (excuse me, my English is not quite well, maybe you misunderstand what I said)
at least, currently:
the caller should be sure that the buffer length is enough (it seems not, I need test it). the internal has no duty to check it.
Signature · 3 lines
Chen Gang Asianux Corporation
Quoted 3 lines · Paul Fulghum
I’m the maintainer for these drivers. I only caught this message by chance and
it seems you are not in MAINTAINER file.
is it suitable to add your name into MAINTAINER file ?
(if it was, please help adding ? I am not quite familiar with it)
thanks.
Signature · 3 lines
Chen Gang Asianux Corporation
Quoted 9 lines · Paul Fulghum
I’m the maintainer for these drivers. I only caught this message by chance and
have not had a chance to review the entire thread and original patches. It’s late and I’m tired so I won’t be able to look at this until tomorrow.I do not doubt there is a problem that needs cleaning up. I just need a day to
review and make sure this does not cause any problems.
if it is surely an issue,
is it suitable to let Paul Fulghum to provide the relative patch ? for synclink, he is more expert than me.
for test and test environments, he is also more expert than me.
thanks.
Signature · 3 lines
Chen Gang Asianux Corporation
Quoted 5 lines · Chen Gang
Quoted 4 lines · Greg KH
Quoted 3 lines · Chen Gang
Quoted 2 lines
And, I really don't understand here, why do you want to change this? What is it going to change? And why?
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Why:
for the contextMGSLPC_INFO*info indrivers/char/pcmcia/synclink_cs.cinfo->max_frame_size can be the value between 4096 .. 65535 (can be set by its module input parameter) info->flag_buf length is 4096 (MAX_ASYNC_BUFFER_SIZE) in function rx_get_frame the framesize is limit by info->max_frame_size, but may still be larger that 4096. when call function ldisc_receive_buf, info->flag_buf is equal to4096, but framesize can be more than 4096. it will cause memory over flow.
The confusion centers on calling the line discipline receive_buf function with a data buffer larger than the flag buffer.
The synclink drivers support asynchronous and synchronous (HDLC) serial communications.
In asynchronous mode, the tty flip buffer is used to feed data to the line discipline. In this mode, the above argument does not apply. The receive_buf function is not called directly.
In synchronous mode, the driver calls the line discipline receive_buf function directly to feed one HDLC frame
of data per call. Maintaining frame boundaries is needed in this mode. This is done only with the N_HDLC line discipline which expects this format and ignores the flag buffer. The flag buffer passed is just a place holder to meet the calling conventions of the line discipline receive_buf function.
The only danger is if:
1. driver is configured for synchronous mode
2. driver is configured for frames > 4K
3. line discipline other than N_HDLC is selected
In this case the line discipline might try to access beyond the end of the flag buffer. This is a non-functional configuration that would not occur on purpose.
Increasing the flag buffer size would prevent a problem in this degenerate case of purposeful misconfiguration. This would be at the expense of larger allocations that are not used.
I think the correct fix is for me to change the direct calls to pass the same buffer for both data and flag and add a comment describing the fact the flag buffer is ignored when using N_HDLC. That way a misconfigured setup won't cause problems and no unneeded allocations are made.
My suggestion is to leave it as is for now until I can make those changes. I admit the current code is ugly enough to cause confusion (sorry Chen Gang), but I don't see any immediate danger.
Signature · 8 lines
Paul Fulghum MicroGate Systems, Ltd. =Customer Driven, by Design= (800)444-1982 (US Sales) (512)345-7791 x102 (Direct) (512)343-9046 (Fax) Central Time Zone (GMT -6h) www.microgate.com
PATCH 1/1 synclink fix ldisc buffer argument
Fix call to line discipline receive_buf by synclink drivers. Dummy flag buffer argument is ignored by N_HDLC line discipline but might be of insufficient size if accessed by a different line discipline selected by mistake. Calls are changed to use data buffer argument for both data and flag buffer so valid memory is provided if the wrong line discipline is used. Unused char_buf and flag_buf are removed.
drivers/char/pcmcia/synclink_cs.c +10 -2
@@ -210,7 +210,6 @@ typedef struct _mgslpc_info {char testing_irq;unsigned int init_error;/* startup error (DIAGS) */ -char flag_buf[MAX_ASYNC_BUFFER_SIZE ];bool drop_rts_on_tx_done;struct _input_signal_events input_signal_events;
@@ -3707,7 +3706,16 @@ static bool rx_get_frame(MGSLPC_INFO *inhdlcdev_rx (info, buf->data, framesize); else#endif -ldisc_receive_buf (tty, buf->data, info->flag_buf, framesize); + { +/* +* Call N_HDLC line discipline directly to maintain +* frame boundaries. Reuse the data buffer argument for the +* flag buffer argument. The flag buffer is ignored by N_HDLC. +* If a different line discipline is selected by mistake it +* will have valid memory for both arguments. +*/ +ldisc_receive_buf (tty, buf->data, buf->data, framesize); + } } }
drivers/tty/synclink.c +20 -4
@@ -291,8 +291,6 @@ struct mgsl_struct {bool lcr_mem_requested;u32 misc_ctrl_value; -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; -char char_buf[MAX_ASYNC_BUFFER_SIZE ];bool drop_rts_on_tx_done;bool loopmode_insert_requested;
@@ -6661,7 +6659,17 @@ static bool mgsl_get_rx_frame(struct mgshdlcdev_rx (info,info->intermediate_rxbuffer,framesize); else#endif -ldisc_receive_buf (tty, info->intermediate_rxbuffer, info->flag_buf, framesize); + { +/* +* Call N_HDLC line discipline directly to maintain +* frame boundaries. Reuse the data buffer argument for the +* flag buffer argument. The flag buffer is ignored by N_HDLC. +* If a different line discipline is selected by mistake it +* will have valid memory for both arguments. +*/ +ldisc_receive_buf (tty, info->intermediate_rxbuffer, + info->intermediate_rxbuffer, framesize); + } } }/* Free the buffers used by this frame. */
@@ -6833,7 +6841,15 @@ static bool mgsl_get_raw_rx_frame(structmemcpy ( info->intermediate_rxbuffer, pBufEntry->virt_addr, framesize); info->icount.rxok++; -ldisc_receive_buf (tty, info->intermediate_rxbuffer, info->flag_buf, framesize); +/* +* Call N_HDLC line discipline directly to maintain +* block boundaries. Reuse the data buffer argument for the +* flag buffer argument. The flag buffer is ignored by N_HDLC. +* If a different line discipline is selected by mistake it +* will have valid memory for both arguments. +*/ +ldisc_receive_buf (tty, info->intermediate_rxbuffer, + info->intermediate_rxbuffer, framesize); }/* Free the buffers used by this frame. */
drivers/tty/synclinkmp.c +11 -4
@@ -262,8 +262,6 @@ typedef struct _synclinkmp_info {bool sca_statctrl_requested;u32 misc_ctrl_value; -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; -char char_buf[MAX_ASYNC_BUFFER_SIZE ];bool drop_rts_on_tx_done;struct _input_signal_events input_signal_events;
@@ -4979,8 +4977,17 @@ CheckAgain:hdlcdev_rx (info,info->tmp_rx_buf,framesize); else#endif -ldisc_receive_buf (tty,info->tmp_rx_buf, - info->flag_buf, framesize); + { +/* +* Call N_HDLC line discipline directly to maintain +* frame boundaries. Reuse the data buffer argument for the +* flag buffer argument. The flag buffer is ignored by N_HDLC. +* If a different line discipline is selected by mistake it +* will have valid memory for both arguments. +*/ +ldisc_receive_buf (tty, info->tmp_rx_buf, + info->tmp_rx_buf, framesize); + } } }/* Free the buffers used by this frame. */
drivers/tty/synclink_gt.c +20 -5
@@ -317,8 +317,6 @@ struct slgt_info {unsigned char *tx_buf;int tx_count; -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; -char char_buf[MAX_ASYNC_BUFFER_SIZE ];bool drop_rts_on_tx_done;struct _input_signal_events input_signal_events;
@@ -4760,7 +4758,16 @@ check_again:hdlcdev_rx (info,info->tmp_rbuf, framesize); else#endif -ldisc_receive_buf (tty, info->tmp_rbuf, info->flag_buf, framesize); + { +/* +* Call N_HDLC line discipline directly to maintain +* frame boundaries. Reuse the data buffer argument for the +* flag buffer argument. The flag buffer is ignored by N_HDLC. +* If a different line discipline is selected by mistake it +* will have valid memory for both arguments. +*/ +ldisc_receive_buf (tty, info->tmp_rbuf, info->tmp_rbuf, framesize); + } } }free_rbufs (info, start, end);
@@ -4793,9 +4800,17 @@ static bool rx_get_buf(struct slgt_info }DBGDATA (info, info->rbufs[i].buf, count,"rx" );DBGINFO (("rx_get_buf size=%d\n" , count)); -if (count) +if (count) { +/* +* Call N_HDLC line discipline directly to maintain +* block boundaries. Reuse the data buffer argument for the +* flag buffer argument. The flag buffer is ignored by N_HDLC. +* If a different line discipline is selected by mistake it +* will have valid memory for both arguments. +*/ ldisc_receive_buf (info->port.tty, info->rbufs[i].buf, - info->flag_buf, count); + info->rbufs[i].buf, count); + }free_rbufs (info, i, i);return true; }
Quoted 4 lines · Paul Fulghum
My suggestion is to leave it as is for now until I can make those changes. I admit the current code is ugly enough to cause confusion (sorry Chen Gang), but I don't see any immediate danger.
do not need 'sorry', learn with each other. (I am just learning
through read kernel source code -- "code review")
I am glad to know that my suggestion is useful (although it seems a
minor suggestion). for me, it is enough.
:-)
thanks.
Signature · 7 lines
Chen Gang Asianux Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] synclink fix ldisc buffer argument
In reply to Paul Fulghumdrivers/char/pcmcia/synclink_cs.c static bool rx_get_frame(MGSLPC_INFO *in · line 3717
Quoted 4 lines
drivers/char/pcmcia/synclink_cs.c +4
-0+* If a different line discipline is selected by mistake it +* will have valid memory for both arguments. +*/ +ldisc_receive_buf (tty, buf->data, buf->data, framesize);
But not valid content it seems
Re: [PATCH] synclink fix ldisc buffer argument
In reply to a message not in the archiveQuoted 4 lines · On Sun, 2 Dec 2012 10:11:58 -0600 Paul Fulghum <[email protected]> wrote:
True, in this mode line disciplines other than
N_HDLCwould not be functional andN_HDLCignores the flag buffer.
This change won’t make other line disciplines useful, it will just prevent the case of a mistakenly selected line discipline accessing beyond the end of the (dummy) flag buffer.I’m fine with or without the change. It is functional now with a chance to read past then end of a buffer if misconfigured. With the change, it has the same functionality without the ability to read past the end of a buffer if misconfigured.
With the change its feeding crap in the flags buffer, which may matter in future depending what happens to the other bits.
If this is a real issue far better to just kzalloc a blank flag buffer to match the mtu.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] synclink fix ldisc buffer argument
In reply to a message not in the archiveQuoted 2 lines · Paul Fulghum
OK, I’ll do that.
pardon (I am just learning)
does 65535 mean HDLC_MAX_FRAME_SIZE ?
why do we need info->max_frame_size >= 4096 ?
in drivers/tty/synclink_gt.c:
3550 if (info->max_frame_size < 4096)
3551 info->max_frame_size = 4096;
3552 else if (info->max_frame_size > 65535)
3553 info->max_frame_size = 65535;
3554
...
3603 info->max_frame_size = 4096;
if possible:
can we move the relative comments (which are inside function) to the
location just above ldisc_receive_buf ?
thanks.
gchen.
Quoted 34 lines
Quoted 20 lines · On Dec 2, 2012, at 12:10 PM, Alan Cox <[email protected]…
Quoted 10 lines · On Sun, 2 Dec 2012 10:11:58 -0600 Paul Fulghum <[email protected]…
True, in this mode line disciplines other than
N_HDLCwould not be functional andN_HDLCignores the flag buffer.
This change won’t make other line disciplines useful, it will just prevent the case of a mistakenly selected line discipline accessing beyond the end of the (dummy) flag buffer.I’m fine with or without the change. It is functional now with a chance to read past then end of a buffer if misconfigured. With the change, it has the same functionality without the ability to read past the end of a buffer if misconfigured.
With the change its feeding crap in the flags buffer, which may matter in future depending what happens to the other bits.
If this is a real issue far better to just kzalloc a blank flag buffer to match the mtu.
Signature · 8 lines
Paul Fulghum MicroGate Systems, Ltd. =Customer Driven, by Design= (800)444-1982 (512)345-7791 (Direct) (512)343-9046 (Fax) Central Time Zone (GMT -5h) www.microgate.com <http://www.microgate.com/>
Signature · 3 lines
Chen Gang Asianux Corporation
Re: [PATCH] synclink fix ldisc buffer argument
Quoted 11 lines · Chen Gang
pardon (I am just learning)
does 65535 meanHDLC_MAX_FRAME_SIZE?
why do we needinfo->max_frame_size>= 4096 ?
indrivers/tty/synclink_gt.c:
3550 if (info->max_frame_size< 4096)
3551info->max_frame_size= 4096;
3552 else if (info->max_frame_size> 65535)
3553info->max_frame_size= 65535;
3554
...
3603info->max_frame_size= 4096;
The hardware can send and receive HDLC frames up to 64K in size. The driver defaults to 4K max frame size to save buffer space for the common case (line 3603 in alloc_dev()).
The module parameter max_frame_size can override the default in add_device() (lines 3550-3554 are from add_device() range checking the module parameter)
Quoted 3 lines
if possible:
can we move the relative comments (which are inside function) to the
location just aboveldisc_receive_buf?
The added comment from my first patch described the reuse of the data buffer as the flag buffer. Alan prefers to use a zero initialized dummy buffer for the flag buffer argument. Doing it that way, the comment is not needed.
Signature · 8 lines
Paul Fulghum MicroGate Systems, Ltd. =Customer Driven, by Design= (800)444-1982 (US Sales) (512)345-7791 x102 (Direct) (512)343-9046 (Fax) Central Time Zone (GMT -6h) www.microgate.com
PATCH 1/1 synclink fix ldisc buffer argument
In reply to Paul FulghumFix call to line discipline receive_buf by synclink drivers. Dummy flag buffer argument is ignored by N_HDLC line discipline but might be of insufficient size if accessed by a different line discipline selected by mistake. flag buffer allocation now matches max size of data buffer. Unused char_buf buffers are removed.
drivers/char/pcmcia/synclink_cs.c +11 -1
@@ -210,7 +210,7 @@ typedef struct _mgslpc_info {char testing_irq;unsigned int init_error;/* startup error (DIAGS) */ -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; +char *flag_buf;bool drop_rts_on_tx_done;struct _input_signal_events input_signal_events;
@@ -2666,6 +2666,14 @@ static int rx_alloc_buffers(MGSLPC_INFOif (info->rx_buf ==NULL )return -ENOMEM ; +/* unused flag buffer to satisfy receive_buf calling interface */ + info->flag_buf =kzalloc (info->max_frame_size,GFP_KERNEL ); +if (!info->flag_buf) { +kfree (info->rx_buf); + info->rx_buf =NULL ; +return -ENOMEM ; + } +rx_reset_buffers (info);return 0 ; }
@@ -2674,6 +2682,8 @@ static void rx_free_buffers(MGSLPC_INFO {kfree (info->rx_buf); info->rx_buf =NULL ; +kfree (info->flag_buf); + info->flag_buf =NULL ; }static int claim_resources (MGSLPC_INFO *info)
drivers/tty/synclink.c +10 -3
@@ -291,8 +291,7 @@ struct mgsl_struct {bool lcr_mem_requested;u32 misc_ctrl_value; -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; -char char_buf[MAX_ASYNC_BUFFER_SIZE ]; +char *flag_buf;bool drop_rts_on_tx_done;bool loopmode_insert_requested;
@@ -3891,7 +3890,13 @@ static int mgsl_alloc_intermediate_rxbuf info->intermediate_rxbuffer =kmalloc (info->max_frame_size,GFP_KERNEL |GFP_DMA );if ( info->intermediate_rxbuffer ==NULL )return -ENOMEM ; - +/* unused flag buffer to satisfy receive_buf calling interface */ + info->flag_buf =kzalloc (info->max_frame_size,GFP_KERNEL ); +if (!info->flag_buf) { +kfree (info->intermediate_rxbuffer); + info->intermediate_rxbuffer =NULL ; +return -ENOMEM ; + }return 0 ; }/* end of mgsl_alloc_intermediate_rxbuffer_memory() */
@@ -3910,6 +3915,8 @@ static void mgsl_free_intermediate_rxbuf {kfree (info->intermediate_rxbuffer); info->intermediate_rxbuffer =NULL ; +kfree (info->flag_buf); + info->flag_buf =NULL ; }/* end of mgsl_free_intermediate_rxbuffer_memory() */
drivers/tty/synclinkmp.c +10 -2
@@ -262,8 +262,7 @@ typedef struct _synclinkmp_info {bool sca_statctrl_requested;u32 misc_ctrl_value; -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; -char char_buf[MAX_ASYNC_BUFFER_SIZE ]; +char *flag_buf;bool drop_rts_on_tx_done;struct _input_signal_events input_signal_events;
@@ -3544,6 +3543,13 @@ static int alloc_tmp_rx_buf(SLMP_INFO *i info->tmp_rx_buf =kmalloc (info->max_frame_size,GFP_KERNEL );if (info->tmp_rx_buf ==NULL )return -ENOMEM ; +/* unused flag buffer to satisfy receive_buf calling interface */ + info->flag_buf =kzalloc (info->max_frame_size,GFP_KERNEL ); +if (!info->flag_buf) { +kfree (info->tmp_rx_buf); + info->tmp_rx_buf =NULL ; +return -ENOMEM ; + }return 0 ; }
@@ -3551,6 +3557,8 @@ static void free_tmp_rx_buf(SLMP_INFO *i {kfree (info->tmp_rx_buf); info->tmp_rx_buf =NULL ; +kfree (info->flag_buf); + info->flag_buf =NULL ; }static int claim_resources (SLMP_INFO *info)
drivers/tty/synclink_gt.c +16 -2
@@ -317,8 +317,7 @@ struct slgt_info {unsigned char *tx_buf;int tx_count; -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; -char char_buf[MAX_ASYNC_BUFFER_SIZE ]; +char *flag_buf;bool drop_rts_on_tx_done;struct _input_signal_events input_signal_events;
@@ -3355,11 +3354,24 @@ static int block_til_ready(struct tty_streturn retval; } +/* +* allocate buffers used for calling line discipline receive_buf +* directly in synchronous mode +* note: add 5 bytes to max frame size to allow appending +* 32-bit CRC and status byte when configured to do so +*/ static int alloc_tmp_rbuf (struct slgt_info *info) { info->tmp_rbuf =kmalloc (info->max_frame_size +5 ,GFP_KERNEL );if (info->tmp_rbuf ==NULL )return -ENOMEM ; +/* unused flag buffer to satisfy receive_buf calling interface */ + info->flag_buf =kzalloc (info->max_frame_size +5 ,GFP_KERNEL ); +if (!info->flag_buf) { +kfree (info->tmp_rbuf); + info->tmp_rbuf =NULL ; +return -ENOMEM ; + }return 0 ; }
@@ -3367,6 +3379,8 @@ static void free_tmp_rbuf(struct slgt_in {kfree (info->tmp_rbuf); info->tmp_rbuf =NULL ; +kfree (info->flag_buf); + info->flag_buf =NULL ; }/*
Re: [PATCH] synclink fix ldisc buffer argument
Quoted 8 lines · Paul Fulghum
Fix call to line discipline
receive_bufby synclink drivers. Dummy flag buffer argument is ignored byN_HDLCline discipline but might be of insufficient size if accessed by a different line discipline selected by mistake. flag buffer allocation now matches max size of data buffer. Unusedchar_bufbuffers are removed.Signed-off-by Paul Fulghum <[email protected]>
thank you very much.
might we use macro instead of hard code number ? (such as 4096,
65535). it is only an idea, not means need regression.
thanks.
Signature · 7 lines
Chen Gang Asianux Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] synclink fix ldisc buffer argument
In reply to Paul FulghumQuoted 13 lines · Paul Fulghum
Quoted 11 lines · Chen Gang
pardon (I am just learning)
does 65535 meanHDLC_MAX_FRAME_SIZE?
why do we needinfo->max_frame_size>= 4096 ?
indrivers/tty/synclink_gt.c:
3550 if (info->max_frame_size< 4096)
3551info->max_frame_size= 4096;
3552 else if (info->max_frame_size> 65535)
3553info->max_frame_size= 65535;
3554
...
3603info->max_frame_size= 4096;
Quoted 9 lines
The hardware can send and receive HDLC frames up to 64K in size. The driver defaults to 4K max frame size to save buffer space for the common case (line 3603 in
alloc_dev()).The module parameter
max_frame_sizecan override the default inadd_device()(lines 3550-3554 are fromadd_device()range checking the module parameter)
thank you.
sorry for reply late (yesterday, I have an annual leave for personal things, and not connect net).
by the way:
does it also need check the length in function rx_get_buf ? (it seems not, but I am not quite sure, can you give a confirm ?)
4779 /*
4780 * pass receive buffer (RAW synchronous mode) to tty layer 4781 * return true if buffer available, otherwise false 4782 */
4783 static bool rx_get_buf(struct slgt_info *info) 4784 {
4785 unsigned int i = info->rbuf_current;
4786 unsigned int count;
4787
4788 if (!desc_complete(info->rbufs[i]))
4789 return false;
4790 count = desc_count(info->rbufs[i]);
4791 switch(info->params.mode) {
4792 case MGSL_MODE_MONOSYNC:
4793 case MGSL_MODE_BISYNC:
4794 case MGSL_MODE_XSYNC:4795 /* ignore residue in byte synchronous modes */
4796 if (desc_residue(info->rbufs[i]))
4797 count--;
4798 break;
4799 }
4800 DBGDATA(info, info->rbufs[i].buf, count, "rx");
4801 DBGINFO(("rx_get_buf size=%d\n", count));
4802 if (count)4803 ldisc_receive_buf(info->port.tty, info->rbufs[i].buf,
4804 info->flag_buf, count);
4805 free_rbufs(info, i, i);
4806 return true;
4807 }
Signature · 7 lines
Chen Gang Asianux Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] synclink fix ldisc buffer argument
In reply to Paul FulghumHello Greg Kroah-Hartman:
Quoted 7 lines · Paul Fulghum
Fix call to line discipline
receive_bufby synclink drivers. Dummy flag buffer argument is ignored byN_HDLCline discipline but might be of insufficient size if accessed by a different line discipline selected by mistake. flag buffer allocation now matches max size of data buffer. Unusedchar_bufbuffers are removed.Signed-off-by Paul Fulghum <[email protected]>
if no additional questions:
is it suitable to let this patch pass checking ?
at least for me, it is surely solve an issue, and no negative effect.
now, maybe Paul Fulghum is busy...
the left questions what I have sent seems minor. so they can be delayed, until he has free time.
when Paul Fulghum has free time.
can still check what my left questions whether valid. if they are valid, can send additional patch for them.
is it OK ?
Regards
Signature · 7 lines
Chen Gang Asianux Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
[Consult]: Re: [PATCH] synclink fix ldisc buffer argument
Hello Paul Fulghum:
Firstly, sorry for my mistake:
I am a reporter (not reviewer), and not suitable to review maintainer's patch. when you send relative patch, need not cc to me (I am not reviewer)
so:
could you send patch again (not need cc to me) ?
(and also it is better to mark me as Reported-by in your patch)
sorry for my mistake, again.
gchen.
Quoted 29 lines · Chen Gang
Hello Greg Kroah-Hartman:
Quoted 7 lines · Paul Fulghum
Fix call to line discipline
receive_bufby synclink drivers. Dummy flag buffer argument is ignored byN_HDLCline discipline but might be of insufficient size if accessed by a different line discipline selected by mistake. flag buffer allocation now matches max size of data buffer. Unusedchar_bufbuffers are removed.Signed-off-by Paul Fulghum <[email protected]>if no additional questions:
is it suitable to let this patch pass checking ?
at least for me, it is surely solve an issue, and no negative effect.now, maybe Paul Fulghum is busy...
the left questions what I have sent seems minor. so they can be delayed, until he has free time.when Paul Fulghum has free time.
can still check what my left questions whether valid. if they are valid, can send additional patch for them.is it OK ?
Regards
Signature · 7 lines
Chen Gang Asianux Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] synclink fix ldisc buffer argument
In reply to Chen GangHello Paul Fulghum:
it seems you are very busy,
and can not get your reply for "checking length in function rx_get_buf".
so I suggest:
design:
to give it additional length checking in function rx_get_buf.
if realy > max_frame_size, will return false (also need call free_rbufs).
unit test plan:
write a simple driver to integrate relative functions (already modified as design). pass compiling, and can succeed loading.
and then
call the relative function (rx_get_buf) with intended values
check the work flow of our modification whether as expected.
at last, I still suggestion Paul Fulghum to provide the patch when he has free time.
for synclink, he is more expert than me.
for synclink, he can give better test.
if still get no reply within an additonal week:
I should provide the patch, it is my duty.
I will be according to the patch which Paul Fulghum has already provided. and then add the "checking length in function rx_get_buf".
Regards
gchen.
Quoted 33 lines · Chen Gang
by the way:
does it also need check the length in functionrx_get_buf? (it seems not, but I am not quite sure, can you give a confirm ?)4779 /*
4780 * pass receive buffer (RAW synchronous mode) to tty layer 4781 * return true if buffer available, otherwise false 4782 */4783 static bool
rx_get_buf(struct slgt_info *info)4784 {
4785 unsigned int i =info->rbuf_current;
4786 unsigned int count;
4787
4788 if (!desc_complete(info->rbufs[i]))
4789 return false;4790 count = desc_count(info->rbufs[i]); 4791 switch(info->params.mode) { 4792 case MGSL_MODE_MONOSYNC: 4793 case MGSL_MODE_BISYNC: 4794 case MGSL_MODE_XSYNC:4795 /* ignore residue in byte synchronous modes */
4796 if (desc_residue(info->rbufs[i]))
4797 count--;
4798 break;4799 } 4800 DBGDATA(info, info->rbufs[i].buf, count, "rx"); 4801 DBGINFO(("rx_get_buf size=%d\n", count)); 4802 if (count)4803 ldisc_receive_buf(info->port.tty, info->rbufs[i].buf,
4804info->flag_buf, count);
4805free_rbufs(info, i, i);
4806 return true;
4807 }
Signature · 7 lines
Chen Gang Asianux Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Greg KH
maintainerRe: [PATCH] synclink fix ldisc buffer argument
Quoted 4 lines · Chen Gang
Hello Paul Fulghum:
it seems you are very busy,
and can not get your reply for "checking length in functionrx_get_buf".
You should always send patches, long emails like this about "potential" issues are hard to handle, patches are best.
greg k-h
Re: [PATCH] synclink fix ldisc buffer argument
Quoted 12 lines · Greg KH
Quoted 4 lines · Chen Gang
Hello Paul Fulghum:
it seems you are very busy,
and can not get your reply for "checking length in functionrx_get_buf".You should always send patches, long emails like this about "potential" issues are hard to handle, patches are best.
greg k-h
ok, I will send patch after unit test.
Signature · 7 lines
Chen Gang Asianux Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/1 drivers/tty/synclink: let receive buffer size match max frame size
In reply to Greg KHBy Paul Fulghum:
Fix call to line discipline receive_buf by synclink drivers. Dummy flag buffer argument is ignored by N_HDLC line discipline but might be of insufficient size if accessed by a different line discipline selected by mistake. flag buffer allocation now matches max size of data buffer. Unused char_buf buffers are removed.
By Chen Gang:
Give a comment for rx_get_buf
the receive buffer size is DMABUFSIZE limited, which alloc in alloc_bufs it is always less than max_frame_size
so do not need check the length whether larger than max_frame_size.
Extern the limitation.
the maxframe parameter (which input from outside) has value limitition so define macro in include/linux/synclink.h to extern the limitation and use macro instead of hard code numbers in all relative c source files
Beautify source code:
for function mgslpc_probe in drivers/char/pcmcia/synclink_cs.c use tab instead of 4 spaces for each line header.
Unit test:
for "Give a comment for rx_get_buf":
only a comment, not need test.
also can reference free_buf, so can confirm our conclusion.
for "Extern the limitation":
modify source code, call the relative function when insmod module.
pass maxframe=6000, for synclink_gt, synclink_cs, synclink, synclinkmp. pass maxframe=3000, for synclink_gt, synclink_cs, synclink, synclinkmp. pass maxframe=70000, for synclink_gt, synclink_cs, synclink, synclinkmp.
for "Beautify source code":
vimdiff the new file and the original file.
restore the new file to the original file, word by word.
make sure that we only use tab instead of 4 spaces for each line header.
--- drivers/char/pcmcia/synclink_cs.c | 102 ++++++++++++++++++++----------------- drivers/tty/synclink.c | 23 ++++++--- drivers/tty/synclink_gt.c | 29 ++++++++--- drivers/tty/synclinkmp.c | 22 +++++--- include/linux/synclink.h | 3 ++ 5 files changed, 111 insertions(+), 68 deletions(-)
drivers/char/pcmcia/synclink_cs.c +56 -46
@@ -210,7 +210,7 @@ typedef struct _mgslpc_info {char testing_irq;unsigned int init_error;/* startup error (DIAGS) */ -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; +char *flag_buf;bool drop_rts_on_tx_done;struct _input_signal_events input_signal_events;
@@ -514,49 +514,49 @@ static const struct tty_port_operations mgslpc_port_ops = {static int mgslpc_probe (struct pcmcia_device *link) { -MGSLPC_INFO *info; -int ret; +MGSLPC_INFO *info; +int ret; -if (debug_level >=DEBUG_LEVEL_INFO ) -printk ("mgslpc_attach\n" ); - - info =kzalloc (sizeof (MGSLPC_INFO ),GFP_KERNEL ); -if (!info) { -printk ("Error can't allocate device instance data\n" ); -return -ENOMEM ; - } - - info->magic =MGSLPC_MAGIC ; -tty_port_init (&info->port); - info->port.ops = &mgslpc_port_ops; -INIT_WORK (&info->task, bh_handler); - info->max_frame_size =4096 ; - info->port.close_delay =5 *HZ /10 ; - info->port.closing_wait =30 *HZ ; -init_waitqueue_head (&info->status_event_wait_q); -init_waitqueue_head (&info->event_wait_q); -spin_lock_init (&info->lock); -spin_lock_init (&info->netlock); -memcpy (&info->params,&default_params,sizeof (MGSL_PARAMS )); - info->idle_mode =HDLC_TXIDLE_FLAGS ; - info->imra_value =0xffff ; - info->imrb_value =0xffff ; - info->pim_value =0xff ; - - info->p_dev = link; - link->priv = info; - -/* Initialize the struct pcmcia_device structure */ - - ret =mgslpc_config (link); -if (ret) { -tty_port_destroy (&info->port); -return ret; - } - -mgslpc_add_device (info); +if (debug_level >=DEBUG_LEVEL_INFO ) +printk ("mgslpc_attach\n" ); -return 0 ; + info =kzalloc (sizeof (MGSLPC_INFO ),GFP_KERNEL ); +if (!info) { +printk ("Error can't allocate device instance data\n" ); +return -ENOMEM ; + } + + info->magic =MGSLPC_MAGIC ; +tty_port_init (&info->port); + info->port.ops = &mgslpc_port_ops; +INIT_WORK (&info->task, bh_handler); + info->max_frame_size =SYNCLINK_MIN_FRAME_SIZE ; + info->port.close_delay =5 *HZ /10 ; + info->port.closing_wait =30 *HZ ; +init_waitqueue_head (&info->status_event_wait_q); +init_waitqueue_head (&info->event_wait_q); +spin_lock_init (&info->lock); +spin_lock_init (&info->netlock); +memcpy (&info->params,&default_params,sizeof (MGSL_PARAMS )); + info->idle_mode =HDLC_TXIDLE_FLAGS ; + info->imra_value =0xffff ; + info->imrb_value =0xffff ; + info->pim_value =0xff ; + + info->p_dev = link; + link->priv = info; + +/* Initialize the struct pcmcia_device structure */ + + ret =mgslpc_config (link); +if (ret) { +tty_port_destroy (&info->port); +return ret; + } + +mgslpc_add_device (info); + +return 0 ; }/* Card has been inserted.
@@ -2674,6 +2674,14 @@ static int rx_alloc_buffers(MGSLPC_INFO *info)if (info->rx_buf ==NULL )return -ENOMEM ; +/* unused flag buffer to satisfy receive_buf calling interface */ + info->flag_buf =kzalloc (info->max_frame_size,GFP_KERNEL ); +if (!info->flag_buf) { +kfree (info->rx_buf); + info->rx_buf =NULL ; +return -ENOMEM ; + } +rx_reset_buffers (info);return 0 ; }
@@ -2682,6 +2690,8 @@ static void rx_free_buffers(MGSLPC_INFO *info) {kfree (info->rx_buf); info->rx_buf =NULL ; +kfree (info->flag_buf); + info->flag_buf =NULL ; }static int claim_resources (MGSLPC_INFO *info)
@@ -2728,10 +2738,10 @@ static void mgslpc_add_device(MGSLPC_INFO *info) current_dev->next_device = info; } -if (info->max_frame_size <4096 ) - info->max_frame_size =4096 ; -else if (info->max_frame_size >65535 ) - info->max_frame_size =65535 ; +if (info->max_frame_size <SYNCLINK_MIN_FRAME_SIZE ) + info->max_frame_size =SYNCLINK_MIN_FRAME_SIZE ; +else if (info->max_frame_size >SYNCLINK_MAX_FRAME_SIZE ) + info->max_frame_size =SYNCLINK_MAX_FRAME_SIZE ;printk ("SyncLink PC Card %s:IO=%04X IRQ=%d\n" , info->device_name, info->io_base, info->irq_level);
drivers/tty/synclink.c +15 -8
@@ -291,8 +291,7 @@ struct mgsl_struct {bool lcr_mem_requested;u32 misc_ctrl_value; -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; -char char_buf[MAX_ASYNC_BUFFER_SIZE ]; +char *flag_buf;bool drop_rts_on_tx_done;bool loopmode_insert_requested;
@@ -3898,7 +3897,13 @@ static int mgsl_alloc_intermediate_rxbuffer_memory(struct mgsl_struct *info) info->intermediate_rxbuffer =kmalloc (info->max_frame_size,GFP_KERNEL |GFP_DMA );if ( info->intermediate_rxbuffer ==NULL )return -ENOMEM ; - +/* unused flag buffer to satisfy receive_buf calling interface */ + info->flag_buf =kzalloc (info->max_frame_size,GFP_KERNEL ); +if (!info->flag_buf) { +kfree (info->intermediate_rxbuffer); + info->intermediate_rxbuffer =NULL ; +return -ENOMEM ; + }return 0 ; }/* end of mgsl_alloc_intermediate_rxbuffer_memory() */
@@ -3917,6 +3922,8 @@ static void mgsl_free_intermediate_rxbuffer_memory(struct mgsl_struct *info) {kfree (info->intermediate_rxbuffer); info->intermediate_rxbuffer =NULL ; +kfree (info->flag_buf); + info->flag_buf =NULL ; }/* end of mgsl_free_intermediate_rxbuffer_memory() */
@@ -4237,10 +4244,10 @@ static void mgsl_add_device( struct mgsl_struct *info ) current_dev->next_device = info; } -if ( info->max_frame_size <4096 ) - info->max_frame_size =4096 ; -else if ( info->max_frame_size >65535 ) - info->max_frame_size =65535 ; +if (info->max_frame_size <SYNCLINK_MIN_FRAME_SIZE ) + info->max_frame_size =SYNCLINK_MIN_FRAME_SIZE ; +else if (info->max_frame_size >SYNCLINK_MAX_FRAME_SIZE ) + info->max_frame_size =SYNCLINK_MAX_FRAME_SIZE ;if ( info->bus_type ==MGSL_BUS_TYPE_PCI ) { printk( "SyncLink PCI v%d %s: IO=%04X IRQ=%d Mem=%08X,%08X MaxFrameSize=%u\n",
@@ -4286,7 +4293,7 @@ static struct mgsl_struct* mgsl_allocate_device(void) info->port.ops = &mgsl_port_ops; info->magic =MGSL_MAGIC ;INIT_WORK (&info->task, mgsl_bh_handler); - info->max_frame_size =4096 ; + info->max_frame_size =SYNCLINK_MIN_FRAME_SIZE ; info->port.close_delay =5 *HZ /10 ; info->port.closing_wait =30 *HZ ;init_waitqueue_head (&info->status_event_wait_q);
drivers/tty/synclink_gt.c +22 -7
@@ -317,8 +317,7 @@ struct slgt_info {unsigned char *tx_buf;int tx_count; -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; -char char_buf[MAX_ASYNC_BUFFER_SIZE ]; +char *flag_buf;bool drop_rts_on_tx_done;struct _input_signal_events input_signal_events;
@@ -3355,11 +3354,24 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,return retval; } +/* +* allocate buffers used for calling line discipline receive_buf +* directly in synchronous mode +* note: add 5 bytes to max frame size to allow appending +* 32-bit CRC and status byte when configured to do so +*/ static int alloc_tmp_rbuf (struct slgt_info *info) { info->tmp_rbuf =kmalloc (info->max_frame_size +5 ,GFP_KERNEL );if (info->tmp_rbuf ==NULL )return -ENOMEM ; +/* unused flag buffer to satisfy receive_buf calling interface */ + info->flag_buf =kzalloc (info->max_frame_size +5 ,GFP_KERNEL ); +if (!info->flag_buf) { +kfree (info->tmp_rbuf); + info->tmp_rbuf =NULL ; +return -ENOMEM ; + }return 0 ; }
@@ -3367,6 +3379,8 @@ static void free_tmp_rbuf(struct slgt_info *info) {kfree (info->tmp_rbuf); info->tmp_rbuf =NULL ; +kfree (info->flag_buf); + info->flag_buf =NULL ; }/*
@@ -3547,10 +3561,10 @@ static void add_device(struct slgt_info *info) current_dev->next_device = info; } -if (info->max_frame_size <4096 ) - info->max_frame_size =4096 ; -else if (info->max_frame_size >65535 ) - info->max_frame_size =65535 ; +if (info->max_frame_size <SYNCLINK_MIN_FRAME_SIZE ) + info->max_frame_size =SYNCLINK_MIN_FRAME_SIZE ; +else if (info->max_frame_size >SYNCLINK_MAX_FRAME_SIZE ) + info->max_frame_size =SYNCLINK_MAX_FRAME_SIZE ;switch (info->pdev->device) {case SYNCLINK_GT_DEVICE_ID :
@@ -3600,7 +3614,7 @@ static struct slgt_info *alloc_dev(int adapter_num, int port_num, struct pci_dev info->port.ops = &slgt_port_ops; info->magic =MGSL_MAGIC ;INIT_WORK (&info->task, bh_handler); - info->max_frame_size =4096 ; + info->max_frame_size =SYNCLINK_MIN_FRAME_SIZE ; info->base_clock =14745600 ; info->rbuf_fill_level =DMABUFSIZE ; info->port.close_delay =5 *HZ /10 ;
@@ -4778,6 +4792,7 @@ cleanup:/* * pass receive buffer (RAW synchronous mode) to tty layer +* the receive buffer size is DMABUFSIZE limited * return true if buffer available, otherwise false */ static bool rx_get_buf (struct slgt_info *info)
drivers/tty/synclinkmp.c +15 -7
@@ -262,8 +262,7 @@ typedef struct _synclinkmp_info {bool sca_statctrl_requested;u32 misc_ctrl_value; -char flag_buf[MAX_ASYNC_BUFFER_SIZE ]; -char char_buf[MAX_ASYNC_BUFFER_SIZE ]; +char *flag_buf;bool drop_rts_on_tx_done;struct _input_signal_events input_signal_events;
@@ -3553,6 +3552,13 @@ static int alloc_tmp_rx_buf(SLMP_INFO *info) info->tmp_rx_buf =kmalloc (info->max_frame_size,GFP_KERNEL );if (info->tmp_rx_buf ==NULL )return -ENOMEM ; +/* unused flag buffer to satisfy receive_buf calling interface */ + info->flag_buf =kzalloc (info->max_frame_size,GFP_KERNEL ); +if (!info->flag_buf) { +kfree (info->tmp_rx_buf); + info->tmp_rx_buf =NULL ; +return -ENOMEM ; + }return 0 ; }
@@ -3560,6 +3566,8 @@ static void free_tmp_rx_buf(SLMP_INFO *info) {kfree (info->tmp_rx_buf); info->tmp_rx_buf =NULL ; +kfree (info->flag_buf); + info->flag_buf =NULL ; }static int claim_resources (SLMP_INFO *info)
@@ -3729,10 +3737,10 @@ static void add_device(SLMP_INFO *info) current_dev->next_device = info; } -if ( info->max_frame_size <4096 ) - info->max_frame_size =4096 ; -else if ( info->max_frame_size >65535 ) - info->max_frame_size =65535 ; +if (info->max_frame_size <SYNCLINK_MIN_FRAME_SIZE ) + info->max_frame_size =SYNCLINK_MIN_FRAME_SIZE ; +else if (info->max_frame_size >SYNCLINK_MAX_FRAME_SIZE ) + info->max_frame_size =SYNCLINK_MAX_FRAME_SIZE ; printk( "SyncLink MultiPort %s: " "Mem=(%08x %08X %08x %08X) IRQ=%d MaxFrameSize=%u\n",
@@ -3773,7 +3781,7 @@ static SLMP_INFO *alloc_dev(int adapter_num, int port_num, struct pci_dev *pdev) info->port.ops = &port_ops; info->magic =MGSL_MAGIC ;INIT_WORK (&info->task, bh_handler); - info->max_frame_size =4096 ; + info->max_frame_size =SYNCLINK_MIN_FRAME_SIZE ; info->port.close_delay =5 *HZ /10 ; info->port.closing_wait =30 *HZ ;init_waitqueue_head (&info->status_event_wait_q);
include/linux/synclink.h +3 -0
@@ -13,6 +13,9 @@#include <uapi/linux/synclink.h> +#define SYNCLINK_MIN_FRAME_SIZE 4096 +#define SYNCLINK_MAX_FRAME_SIZE 65535 +/* provide 32 bit ioctl compatibility on 64 bit systems */ #ifdef CONFIG_COMPAT #include <linux/compat.h>
Signature · 1 line
1.7.10.4