[Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[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 #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_receivebuf
tty/n_r3964.c:156:	.receive_buf = r3964_receive_buf,

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Alan Cox]

On Thu, 29 Nov 2012 13:07:28 +0800
Chen Gang <gang.chen@asianux.com> 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 #define MAX_ASYNC_BUFFER_SIZE 0x10000  (better than 0xffff)

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 ?

> 
> ---------------------------------------------------------------------------------
> 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.

Why should it - MAX_ASYNC_BUFFER_SIZE is an internal detail of the
synclink drivers. 

Alan

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Chen Gang]

于 2012年11月29日 21:41, Alan Cox 写道:
> On Thu, 29 Nov 2012 13:07:28 +0800
> Chen Gang <gang.chen@asianux.com> 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 #define MAX_ASYNC_BUFFER_SIZE 0x10000  (better than 0xffff)
> 
> 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];


>>
>> ---------------------------------------------------------------------------------
>> 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.
> 
> Why should it - MAX_ASYNC_BUFFER_SIZE is 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.


-- 
Chen Gang

Asianux Corporation

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Chen Gang]

于 2012年11月30日 10:27, Chen Gang 写道:
> 于 2012年11月29日 21:41, Alan Cox 写道:
>> On Thu, 29 Nov 2012 13:07:28 +0800
>> Chen Gang <gang.chen@asianux.com> 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 #define MAX_ASYNC_BUFFER_SIZE 0x10000  (better than 0xffff)
>>
>> 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.
> 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];
> 
> 
>>>
>>> ---------------------------------------------------------------------------------
>>> 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.
>>
>> Why should it - MAX_ASYNC_BUFFER_SIZE is 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.
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: 【Suggestion】drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Greg KH]

On Thu, Nov 29, 2012 at 12:40:49PM +0800, Chen Gang 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 #define MAX_ASYNC_BUFFER_SIZE 0x10000  (better than 0xffff)

Please, send tty questions to the linux-serial@vger.kernel.org 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

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Chen Gang]

于 2012年11月29日 13:13, Greg KH 写道:
> On Thu, Nov 29, 2012 at 12:40:49PM +0800, Chen Gang 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 #define MAX_ASYNC_BUFFER_SIZE 0x10000  (better than 0xffff)
> 
> Please, send tty questions to the linux-serial@vger.kernel.org list
> also.

  I cc to linux-serial@vger.kernel.org, in this reply.

  I referenced the file MAINTAINERS, before sent original mail:

  it seems all drivers/tty/serial/* are relative with
linux-serial@vger.kernel.org. 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 linux-kernel@vger.kernel.org.

  next time, for all tty questions, I will cc to
linux-serial@vger.kernel.org (not cc to linux-kernel@vger.kernel.org).

> 
> 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 to
4096, 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.

> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 


-- 
Chen Gang

Asianux Corporation

[PATCH] MAINTAINERS: TTY - Add linux-serial mailing list

[Joe Perches]

Signed-off-by: Joe Perches <joe@perches.com>
---
 MAINTAINERS |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index afc0b27..255dafb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7658,6 +7658,7 @@ K:	^Subject:.*(?i)trivial
 TTY LAYER
 M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 M:	Jiri Slaby <jslaby@suse.cz>
+L:	linux-serial@vger.kernel.org
 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

[Chen Gang]

于 2012年11月29日 14:14, Joe Perches 写道:
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  MAINTAINERS |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index afc0b27..255dafb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7658,6 +7658,7 @@ K:	^Subject:.*(?i)trivial
>  TTY LAYER
>  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>  M:	Jiri Slaby <jslaby@suse.cz>
> +L:	linux-serial@vger.kernel.org
>  S:	Supported
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
>  F:	drivers/tty/
> 
> 
> 
> 

  thank you very much.


-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] MAINTAINERS: TTY - Add linux-serial mailing list

[Chen Gang]

于 2012年11月29日 14:14, Joe Perches 写道:
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  MAINTAINERS |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index afc0b27..255dafb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7658,6 +7658,7 @@ K:	^Subject:.*(?i)trivial
>  TTY LAYER
>  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>  M:	Jiri Slaby <jslaby@suse.cz>
> +L:	linux-serial@vger.kernel.org
>  S:	Supported
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
>  F:	drivers/tty/
> 
> 
> 
> 

  thank you very much.


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] MAINTAINERS: TTY - Add linux-serial mailing list

[Jiri Slaby]

On 11/29/2012 07:14 AM, Joe Perches wrote:
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  MAINTAINERS |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index afc0b27..255dafb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7658,6 +7658,7 @@ K:	^Subject:.*(?i)trivial
>  TTY LAYER
>  M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>  M:	Jiri Slaby <jslaby@suse.cz>
> +L:	linux-serial@vger.kernel.org

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,
-- 
js
suse labs

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Greg KH]

On Thu, Nov 29, 2012 at 01:57:59PM +0800, Chen Gang wrote:
> > 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 to
> 4096, 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?

> 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.

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

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Chen Gang]

于 2012年11月30日 02:32, Greg KH 写道:
> On Thu, Nov 29, 2012 at 01:57:59PM +0800, Chen Gang wrote:
>>> 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 to
>> 4096, 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?

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];



> 
>> 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.
> 
> 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.



> thanks,
> 
> greg k-h
> 
> 


-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Chen Gang]

于 2012年11月30日 02:32, Greg KH 写道:
> On Thu, Nov 29, 2012 at 01:57:59PM +0800, Chen Gang wrote:
>>> 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 to
>> 4096, 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?

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];



> 
>> 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.
> 
> 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.



> thanks,
> 
> greg k-h
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Chen Gang]

于 2012年11月30日 11:27, 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.

-- 
Chen Gang

Asianux Corporation

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Chen Gang]

于 2012年11月30日 11:27, 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.

-- 
Chen Gang

Asianux Corporation

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Paul Fulghum]

On 11/29/2012 8:52 PM, Chen Gang wrote:
> 于 2012年11月30日 02:32, Greg KH 写道:
>> On Thu, Nov 29, 2012 at 01:57:59PM +0800, Chen Gang wrote:
>>>> 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 to
>>> 4096, 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.

-- 
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] synclink fix ldisc buffer argument

[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. 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.

Signed-off-by: Paul Fulghum <paulkf@microgate.com>


--- a/drivers/char/pcmcia/synclink_cs.c	2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/char/pcmcia/synclink_cs.c	2012-11-30 12:50:23.000000000 -0600
@@ -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 *in
 				hdlcdev_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);
+			}
 		}
 	}
 
--- a/drivers/tty/synclink.c	2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclink.c	2012-11-30 12:59:29.000000000 -0600
@@ -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 mgs
 				hdlcdev_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(struct
 			memcpy( 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. */
--- a/drivers/tty/synclinkmp.c	2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclinkmp.c	2012-11-30 13:01:36.000000000 -0600
@@ -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. */
--- a/drivers/tty/synclink_gt.c	2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclink_gt.c	2012-11-30 12:53:25.000000000 -0600
@@ -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;
 }

Re: [PATCH] synclink fix ldisc buffer argument

[Alan Cox]

> +				 * 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

[Alan Cox]

On Sun, 2 Dec 2012 10:11:58 -0600
Paul Fulghum <paulkf@microgate.com> wrote:

> True, in this mode line disciplines other than N_HDLC would not be functional and N_HDLC ignores 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] synclink fix ldisc buffer argument

[Alan Cox]

On Sun, 2 Dec 2012 10:11:58 -0600
Paul Fulghum <paulkf@microgate.com> wrote:

> True, in this mode line disciplines other than N_HDLC would not be functional and N_HDLC ignores 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.

Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

于 2012年12月03日 04:05, 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.


> On Dec 2, 2012, at 12:10 PM, Alan Cox <alan@lxorguk.ukuu.org.uk
> <mailto:alan@lxorguk.ukuu.org.uk>> wrote:
> 
>> On Sun, 2 Dec 2012 10:11:58 -0600
>> Paul Fulghum <paulkf@microgate.com <mailto:paulkf@microgate.com>> wrote:
>>
>>> True, in this mode line disciplines other than N_HDLC would not be
>>> functional and N_HDLC ignores 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.
>>
> 
> -- 
> 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/>
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] synclink fix ldisc buffer argument

[Paul Fulghum]

On 12/2/2012 8:20 PM, Chen Gang wrote:
> 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;

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)

> if possible:
>   can we move the relative comments (which are inside function) to the
> location just above ldisc_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.

-- 
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

Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

于 2012年12月04日 00:03, Paul Fulghum 写道:
> On 12/2/2012 8:20 PM, Chen Gang wrote:
>> 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;
> 
> 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)
> 

  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 }




-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

于 2012年12月04日 00:03, Paul Fulghum 写道:
> On 12/2/2012 8:20 PM, Chen Gang wrote:
>> 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;
> 
> 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)
> 

  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 }




-- 
Chen Gang

Asianux Corporation

Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

Hello 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.

于 2012年12月05日 09:57, Chen Gang 写道:
> 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 }


-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

Hello 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.

于 2012年12月05日 09:57, Chen Gang 写道:
> 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 }


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] synclink fix ldisc buffer argument

[Greg KH]

On Wed, Dec 19, 2012 at 10:23:29AM +0800, Chen Gang wrote:
> Hello Paul Fulghum:
> 
> it seems you are very busy,
>   and can not get your reply for "checking length in function rx_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

[Chen Gang]

于 2012年12月19日 12:09, Greg KH 写道:
> On Wed, Dec 19, 2012 at 10:23:29AM +0800, Chen Gang wrote:
>> Hello Paul Fulghum:
>>
>> it seems you are very busy,
>>   and can not get your reply for "checking length in function rx_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.


-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

于 2012年12月19日 12:09, Greg KH 写道:
> On Wed, Dec 19, 2012 at 10:23:29AM +0800, Chen Gang wrote:
>> Hello Paul Fulghum:
>>
>> it seems you are very busy,
>>   and can not get your reply for "checking length in function rx_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.


-- 
Chen Gang

Asianux Corporation

[PATCH] drivers/tty/synclink: let receive buffer size match max frame size

[Chen Gang]

By 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.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
Signed-off-by: Paul Fulghum <paulkf@microgate.com>
---
 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(-)

diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index b66eaa0..4f78e30 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -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);
diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c
index 9e071f6..94ce422 100644
--- a/drivers/tty/synclink.c
+++ b/drivers/tty/synclink.c
@@ -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);
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index aba1e59..4dda746 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -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)
diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c
index fd43fb6..621c562 100644
--- a/drivers/tty/synclinkmp.c
+++ b/drivers/tty/synclinkmp.c
@@ -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);
diff --git a/include/linux/synclink.h b/include/linux/synclink.h
index f1405b1..61ac4b2 100644
--- a/include/linux/synclink.h
+++ b/include/linux/synclink.h
@@ -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>
-- 
1.7.10.4

[PATCH] synclink fix ldisc buffer argument

[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.

Signed-off-by: Paul Fulghum <paulkf@microgate.com>

--- a/drivers/char/pcmcia/synclink_cs.c	2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/char/pcmcia/synclink_cs.c	2012-12-03 10:51:40.000000000 -0600
@@ -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_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;
 }
@@ -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)
--- a/drivers/tty/synclink.c	2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclink.c	2012-12-03 10:51:56.000000000 -0600
@@ -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() */
 
--- a/drivers/tty/synclinkmp.c	2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclinkmp.c	2012-12-03 10:52:09.000000000 -0600
@@ -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)
--- a/drivers/tty/synclink_gt.c	2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclink_gt.c	2012-12-03 11:00:15.000000000 -0600
@@ -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_st
 	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_in
 {
 	kfree(info->tmp_rbuf);
 	info->tmp_rbuf = NULL;
+	kfree(info->flag_buf);
+	info->flag_buf = NULL;
 }
 
 /*

Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

于 2012年12月04日 01:13, 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.
> 
> Signed-off-by: Paul Fulghum <paulkf@microgate.com>
> 

  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.

-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

于 2012年12月04日 01:13, 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.
> 
> Signed-off-by: Paul Fulghum <paulkf@microgate.com>
> 

  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.

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

Hello Greg Kroah-Hartman:


于 2012年12月04日 01:13, 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.
> 
> Signed-off-by: Paul Fulghum <paulkf@microgate.com>

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

-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

Hello Greg Kroah-Hartman:


于 2012年12月04日 01:13, 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.
> 
> Signed-off-by: Paul Fulghum <paulkf@microgate.com>

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

-- 
Chen Gang

Asianux Corporation

[Consult]: Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

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.


于 2012年12月07日 10:15, Chen Gang 写道:
> Hello Greg Kroah-Hartman:
> 
> 
> 于 2012年12月04日 01:13, 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.
>>
>> Signed-off-by: Paul Fulghum <paulkf@microgate.com>
> 
> 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
> 


-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Consult]: Re: [PATCH] synclink fix ldisc buffer argument

[Chen Gang]

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.


于 2012年12月07日 10:15, Chen Gang 写道:
> Hello Greg Kroah-Hartman:
> 
> 
> 于 2012年12月04日 01:13, 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.
>>
>> Signed-off-by: Paul Fulghum <paulkf@microgate.com>
> 
> 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
> 


-- 
Chen Gang

Asianux Corporation

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Chen Gang]

于 2012年12月01日 00:24, 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.

-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE

[Chen Gang]

于 2012年12月01日 00:24, 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.

-- 
Chen Gang

Asianux Corporation