Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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 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 0x1 (better than 0x) >> >> 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 0x1 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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 0x1 (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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 2012年11月29日 21:41, Alan Cox 写道: > On Thu, 29 Nov 2012 13:07:28 +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 0x1 (better than 0x) > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
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 0x1 (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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
On Thu, 29 Nov 2012 13:07:28 +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 0x1 (better than 0x) 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
On Thu, 29 Nov 2012 13:07:28 +0800 Chen Gang gang.c...@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 0x1 (better than 0x) 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
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 0x1 (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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 2012年11月29日 21:41, Alan Cox 写道: On Thu, 29 Nov 2012 13:07:28 +0800 Chen Gang gang.c...@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 0x1 (better than 0x) 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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 0x1 (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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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.c...@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 0x1 (better than 0x) 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 0x1 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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 0x1 (better than 0x) > > Please, send tty questions to the linux-ser...@vger.kernel.org list > also. I cc to linux-ser...@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-ser...@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-ser...@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 0x1 (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 majord...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 【Suggestion】drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
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 0x1 (better than 0x) Please, send tty questions to the linux-ser...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
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 0x1 (better than 0x)
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
[Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
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 0x1 (better than 0x)
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
Re: 【Suggestion】drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
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 0x1 (better than 0x) Please, send tty questions to the linux-ser...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Suggestion] drivers/tty: drivers/char/: for MAX_ASYNC_BUFFER_SIZE
于 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 0x1 (better than 0x) Please, send tty questions to the linux-ser...@vger.kernel.org list also. I cc to linux-ser...@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-ser...@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-ser...@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 0x1 (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 majord...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
