Re: MLEN and crashes
On 2000-Apr-14 23:40:53 +1000, Poul-Henning Kamp <[EMAIL PROTECTED]> wrote: >In message <[EMAIL PROTECTED]>, Peter Jeremy write >s: >>Many years ago, I wrote a tool that analysed stack requirements by >>parsing the assembler output from the compiler. ... >Commit it either as a general tool or as a kernel targeted tool >under src/tools. And the faster the better :-) I'll have to see if I can still find it. In any case, it was designed to handle M68K assembler from a Microtec compiler. It would need some re-work before it could work with FreeBSD. On 2000-Apr-14 23:49:58 +1000, Thomas David Rivers <[EMAIL PROTECTED]> wrote: > How did you address recursive functions, or were they also forbidden > by design standards? They were forbidden from memory. (Which will be a nuisance here). They would show up as loops in the call graph. Even if I can't find my previous code, the design of such a tool is fairly trivial (given the assembler, or a suitably patched compiler). The first part reads the assembler: When a function is defined, you note the local framesize and keep track of other stack pushes/pops to work out total stack requirements for the function. When a function is called, you output a triplet of caller, called function and stack usage. (From memory I cheated a bit and output caller/called pairs, with a separate maximum stack used line). This information should be fairly trivially available within the compiler as an alternative. The second part reads all the output from the first part and forms a call flow graph, generating maximum stack needs from each node. The top level node(s) give you the total stack needs. One reason for splitting it into two bits is to make it easier to add manual entries for indirect function calls. (Some of this `manual' work could be partially automated). Recursive calls (which there are a fair number of in the FreeBSD kernel) aren't that easily handled and would probably entail manual (or semi-automatic) editing. I'll have a go at a more detailed design (and see if I can find or re-implement it) if no-one else has one already. Peter To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
Peter Jeremy <[EMAIL PROTECTED]> wrote: > > On 3/04, John Polstra wrote: > [don't allocate big structs on kernel stack] > > Many years ago, I wrote a tool that analysed stack requirements by > parsing the assembler output from the compiler. It determined the > stack frame requirements and built a call flow graph to determine > total stack depth. It had some hooks to allow indirect function > calls to be specified manually. It couldn't handle alloca() (and > equivalents), but they were forbidden by the design standards. Just wondering... How did you address recursive functions, or were they also forbidden by design standards? - Dave Rivers - To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
In message <[EMAIL PROTECTED]>, Peter Jeremy write s: >Many years ago, I wrote a tool that analysed stack requirements by >parsing the assembler output from the compiler. It determined the >stack frame requirements and built a call flow graph to determine >total stack depth. It had some hooks to allow indirect function >calls to be specified manually. It couldn't handle alloca() (and >equivalents), but they were forbidden by the design standards. > > >What are other people's opinions on the usefulness of something >like this? Commit it either as a general tool or as a kernel targeted tool under src/tools. And the faster the better :-) -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 [EMAIL PROTECTED] | TCP/IP since RFC 956 FreeBSD coreteam member | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
On 3/04, John Polstra wrote: [don't allocate big structs on kernel stack] Many years ago, I wrote a tool that analysed stack requirements by parsing the assembler output from the compiler. It determined the stack frame requirements and built a call flow graph to determine total stack depth. It had some hooks to allow indirect function calls to be specified manually. It couldn't handle alloca() (and equivalents), but they were forbidden by the design standards. Anyone who does embedded work probably has something similar (or maybe better). It occurs to me that something along these lines would be quite useful for picking overly expensive (in stack space) subroutine threads within the kernel. (And maybe identifying areas where we could safely allow malloc'd structs to be made auto). The downside is that indirect function calls would need to be manually identified - which could be a significant amount of effort. What are other people's opinions on the usefulness of something like this? Peter To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
Gary Jennejohn wrote: > Peter Wemm writes: > >Gary Jennejohn wrote: > >> Bruce Evans writes: > >> >On Mon, 3 Apr 2000, Gary Jennejohn wrote: > >> > > >> >> Bruce Evans writes: > >> >> >On Sun, 2 Apr 2000, Gary Jennejohn wrote: > >> >> >> I think we should nuke csu_hdr since it's not used anywhere. Is that > >> >> >> what you really mean ? > >> >> > > >> >> >Yes. I'm trying the following patch. Only tested at compile time. > >> >> > > >> >> [patch snipped] > >> >> > >> >> Thank you, Bruce ! This is pretty much the same patch I tested. > >> >> > >> >> So, should I commit it ? > >> > > >> >If you have tested it :-). > >> > > >> > >> I'm running with the change right now. No problems. > > > >I would prefer that we did this: > > > >#define MAX_LEN (min(128, MLEN)) > > > >or something like that. This should stop Bad Things happening if > >somebody recompiles with MLEN set specifically to 128 (and is an ideal > >MFC candidate for 4.x for when people set MLEN to 256 over there). > > > > This is a pretty good idea, too. But I already deleted csu_hdr in -current. > It would be easy enough to back out the change if there's consensus. No, leave csu_hdr deleted. I am only talking about also changing the #define MAX_LEN to something that's safe even if MLEN is overridden. Cheers, -Peter To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
Peter Wemm writes: >Gary Jennejohn wrote: >> Bruce Evans writes: >> >On Mon, 3 Apr 2000, Gary Jennejohn wrote: >> > >> >> Bruce Evans writes: >> >> >On Sun, 2 Apr 2000, Gary Jennejohn wrote: >> >> >> I think we should nuke csu_hdr since it's not used anywhere. Is that >> >> >> what you really mean ? >> >> > >> >> >Yes. I'm trying the following patch. Only tested at compile time. >> >> > >> >> [patch snipped] >> >> >> >> Thank you, Bruce ! This is pretty much the same patch I tested. >> >> >> >> So, should I commit it ? >> > >> >If you have tested it :-). >> > >> >> I'm running with the change right now. No problems. > >I would prefer that we did this: > >#define MAX_LEN (min(128, MLEN)) > >or something like that. This should stop Bad Things happening if >somebody recompiles with MLEN set specifically to 128 (and is an ideal >MFC candidate for 4.x for when people set MLEN to 256 over there). > This is a pretty good idea, too. But I already deleted csu_hdr in -current. It would be easy enough to back out the change if there's consensus. --- Gary Jennejohn / [EMAIL PROTECTED] [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
Gary Jennejohn wrote: > Bruce Evans writes: > >On Mon, 3 Apr 2000, Gary Jennejohn wrote: > > > >> Bruce Evans writes: > >> >On Sun, 2 Apr 2000, Gary Jennejohn wrote: > >> >> I think we should nuke csu_hdr since it's not used anywhere. Is that > >> >> what you really mean ? > >> > > >> >Yes. I'm trying the following patch. Only tested at compile time. > >> > > >> [patch snipped] > >> > >> Thank you, Bruce ! This is pretty much the same patch I tested. > >> > >> So, should I commit it ? > > > >If you have tested it :-). > > > > I'm running with the change right now. No problems. I would prefer that we did this: #define MAX_LEN (min(128, MLEN)) or something like that. This should stop Bad Things happening if somebody recompiles with MLEN set specifically to 128 (and is an ideal MFC candidate for 4.x for when people set MLEN to 256 over there). Cheers, -Peter -- Peter Wemm - [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED] "All of this is for nothing if we don't go to the stars" - JMS/B5 To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
Bruce Evans writes: >On Mon, 3 Apr 2000, Gary Jennejohn wrote: > >> Bruce Evans writes: >> >On Sun, 2 Apr 2000, Gary Jennejohn wrote: >> >> I think we should nuke csu_hdr since it's not used anywhere. Is that >> >> what you really mean ? >> > >> >Yes. I'm trying the following patch. Only tested at compile time. >> > >> [patch snipped] >> >> Thank you, Bruce ! This is pretty much the same patch I tested. >> >> So, should I commit it ? > >If you have tested it :-). > I'm running with the change right now. No problems. --- Gary Jennejohn / [EMAIL PROTECTED] [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
On Mon, 3 Apr 2000, Gary Jennejohn wrote: > Bruce Evans writes: > >On Sun, 2 Apr 2000, Gary Jennejohn wrote: > >> I think we should nuke csu_hdr since it's not used anywhere. Is that > >> what you really mean ? > > > >Yes. I'm trying the following patch. Only tested at compile time. > > > [patch snipped] > > Thank you, Bruce ! This is pretty much the same patch I tested. > > So, should I commit it ? If you have tested it :-). Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
On 3/04, John Polstra wrote:
| I doubt if it's possible to implement that at compile time. Remember,
| the preprocessor doesn't understand "sizeof". It doesn't recognize
| keywords in expressions at all.
Then don't use the preprocessor alone and use both the preprocessor and
the compiler. I suppose something like this will work:
struct foobar {
...;
}
#ifdef CHECK_STRUCT_SIZE
#define MAX_FOOBAR_SIZE 8
static char dummy_foobar [MAX_FOOBAR_SIZE-sizeof(struct foobar)];
#undef MAX_FOOBAR_SIZE
#endif
If sizeof(struct foobar) is more than MAX_FOOBAR_SIZE, then the compiler
will try to create an array with a negative size, which will not compile.
Embed this in a macro and you're done:
#ifdef CHECK_STRUCT_SIZE
#define CSS(S,T,U) static char dummy_##T [U-sizeof(S)];
#else
#define CSS(S,T,U)
#endif
Then use:
CSS(struct foobar,foobar,8)
Sam
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
In article <[EMAIL PROTECTED]>, Alfred Perlstein <[EMAIL PROTECTED]> wrote: > > If you're worried about such things happening then you can use > the pre-processor to catch things that may make your structures > too large. > > > I wonder how too "big" can be detected. The code in question is perfectly > > valid syntactically and semantically correct C-code. > > There's code that #error (maybe #warning?) when it can detect that the > size of a struct has been unreasonably grown, it's done via the > pre-processor. I doubt if it's possible to implement that at compile time. Remember, the preprocessor doesn't understand "sizeof". It doesn't recognize keywords in expressions at all. John -- John Polstra [EMAIL PROTECTED] John D. Polstra & Co., Inc.Seattle, Washington USA "Disappointment is a good sign of basic intelligence." -- Chögyam Trungpa To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
Poul-Henning Kamp wrote: > In message <[EMAIL PROTECTED]>, Gary Jennejohn writes: > > >Yes, but it was perfectly legal to put the structure on the stack > >_before_ MLEN was doubled. > > Just because it worked doesn't mean that it was correct. > > We need to be frugal about the kernel stack, for a lot of reasons, > that's just the way it is, and as far as I know it is the way > it will continue to be. To be a little less harsh about it (Poul, you could _really_ stand to work on your phrasing, this sounded unnecessarily harsh), kernel stack space is (in pretty much any kernel -- the one I work on is Unixware 7) a very scarce resource. You can put variables on the stack, but even that can make it run out for really long path lengths. If you have a structure or an array, the rule of thumb is to _always_ malloc it. This will save your bacon when the structure grows (as in this case) or the path gets a little longer, or something else happens that makes you run out of kernel stack. This is the way it is in kernel land. I won't say "get used to it," that's unnecessarily harsh, but it _is_ the way it is. Read, learn, evolve, as they say in talk.bizarre. And become a better kernel programmer thereby. -- Frank Mayhar [EMAIL PROTECTED] http://www.exit.com/ To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
Bruce Evans writes: >On Sun, 2 Apr 2000, Gary Jennejohn wrote: >> I think we should nuke csu_hdr since it's not used anywhere. Is that >> what you really mean ? > >Yes. I'm trying the following patch. Only tested at compile time. > [patch snipped] Thank you, Bruce ! This is pretty much the same patch I tested. So, should I commit it ? --- Gary Jennejohn / [EMAIL PROTECTED] [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
Hellmuth Michaelis wrote: > > >From the keyboard of Poul-Henning Kamp: > > > We need to be frugal about the kernel stack, for a lot of reasons, > > that's just the way it is, and as far as I know it is the way > > it will continue to be. > > Good. I'd like to learn something from it: Shall i avoid allocating > structs on the kernel stack at all or is it just bad to allocate > big structs ? If the latter is true, what number is big > The best rule is never allocate anything bigger than a few elements. One reason to not allocate struct on the stack is that under some OS's (e.g. originally freebsd too) the stack could have been 'unmapped' while the that process was not in core, tus access toteh struct by an interrupt routine or something could be 'bad'. In FreeBSD this is presently not the case but.. Also, when we switch to ASYNC IO as a normal means of operation, the stack may sometimes be rewound before the IO is completed, which might also be considered "not a good thing" (TM). > -- __--_|\ Julian Elischer / \ [EMAIL PROTECTED] ( OZ) World tour 2000 ---> X_.---._/ presently in: Perth v To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
On Mon, 3 Apr 2000, Alfred Perlstein wrote: > * Hellmuth Michaelis <[EMAIL PROTECTED]> [000403 02:12] wrote: > > >From the keyboard of Bruce Evans: > > > > > It's just a bug to allocate big structs on the kernel stack. > > > > Please specify "big"! :-) > > have a look at src/sys/nfs/nfs_vnops.c: > > line ~2787: > #ifndef NFS_COMMITBVECSIZ > #define NFS_COMMITBVECSIZ 20 > #endif > struct buf *bvec_on_stack[NFS_COMMITBVECSIZ]; > int bvecsize = 0, bveccount; > > I guess 80 bytes is pushing it, some routines are worse, but 2k > is totally out of line. This case is an array of pointers. Big arrays are just as bad as big structs, of course. Having a parameterized array size makes it hard to prove that the size is small enough. > If you're worried about such things happening then you can use > the pre-processor to catch things that may make your structures > too large. This won't work for dynamic arrays. E.g., vm_pageout_flush() uses "int pageout_status[count];". It is not immediately obvious that count < vm_pageout_page_count where vm_pageout_page_count is either 8 or VM_PAGEOUT_PAGE_COUNT = 16. Using a constant array size of VM_PAGEOUT_PAGE_COUNT would be simpler, faster, and wouldn't break K&R support. objdump(1) can be used to find struct sizes. E.g., "objdump --stabs kernel.debug | grep cstate" gives: 18052 LCSYM 0 120c02b7a00 208921 escstate:V(0,1) 129365 LSYM 0 0 1929789 cstate:T(55,1)=s244cs_next:(55,2)=*(55,1),0,32;cs_hlen:(7,8),32,16;cs_id:(7,1),48,8;cs_filler:(7,1),56,8;slcs_u:(55,3)=u236csu_hdr:(35,14),0,1888;csu_ip:(54,1),0,160;;,64,1888;; This shows that the last struct member has size 64 bits and offset 1888 bits. The struct size is usually the sum of the last member offset and size. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
In message <[EMAIL PROTECTED]>, Hellmuth Michaelis writes : >>From the keyboard of Poul-Henning Kamp: > >> We need to be frugal about the kernel stack, for a lot of reasons, >> that's just the way it is, and as far as I know it is the way >> it will continue to be. > >Good. I'd like to learn something from it: Shall i avoid allocating >structs on the kernel stack at all or is it just bad to allocate >big structs ? My own rule of thumb is "about 60 bytes or so", but it also depends on the lifetime of the function. If it is a leaf function which doesn't call anything else I'll let it use more stack, if it is a function which is burried at the bottom (top really) of the stack all the time I'm less tolerant. -- Poul-Henning Kamp FreeBSD coreteam member [EMAIL PROTECTED] "Real hackers run -current on their laptop." FreeBSD -- It will take a long time before progress goes too far! To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
>From the keyboard of Poul-Henning Kamp: > We need to be frugal about the kernel stack, for a lot of reasons, > that's just the way it is, and as far as I know it is the way > it will continue to be. Good. I'd like to learn something from it: Shall i avoid allocating structs on the kernel stack at all or is it just bad to allocate big structs ? If the latter is true, what number is big ? I've scanned a bit through the kernel sources to find the constant which defines the length of the kernel stack but i was not able to find anything (which then could be used at compile time to detect a potentially too large struct). > Get used to it. Will do ;-) hellmuth -- Hellmuth MichaelisTel +49 40 55 97 47-70 HCS Hanseatischer Computerservice GmbHFax +49 40 55 97 47-77 Oldesloer Strasse 97-99 Mail hm [at] hcs.de D-22457 Hamburg WWW http://www.hcs.de To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
On Sun, 2 Apr 2000, Gary Jennejohn wrote:
> Bruce Evans writes:
> >Big structs need to be malloced.
>
> Yes, but how does one know that a struct is too big ? Before the increase
> in MLEN strucct sppp was not too big.
All structs should be considered too big until proven otherwise :-).
> >I think removing the unused bloat in `struct cstate' is the correct fix
> >for this particular allocation.
> >
>
> I think we should nuke csu_hdr since it's not used anywhere. Is that
> what you really mean ?
Yes. I'm trying the following patch. Only tested at compile time.
diff -c2 slcompress.h~ slcompress.h
*** slcompress.h~ Sun Aug 29 13:15:00 1999
--- slcompress.hSun Apr 2 21:53:35 2000
***
*** 41,46
#define _NET_SLCOMPRESS_H_
! #define MAX_STATES 16 /* must be > 2 and < 256 */
! #define MAX_HDR MLEN /* XXX 4bsd-ism: should really be 128 */
/*
--- 41,46
#define _NET_SLCOMPRESS_H_
! #define MAX_STATES16 /* must be > 2 and < 256 */
! #define MAX_HDR 128
/*
***
*** 120,130
u_char cs_id; /* connection # associated with this state */
u_char cs_filler;
! union {
! char csu_hdr[MAX_HDR];
! struct ip csu_ip; /* ip/tcp hdr from most recent packet */
! } slcs_u;
};
- #define cs_ip slcs_u.csu_ip
- #define cs_hdr slcs_u.csu_hdr
/*
--- 120,125
u_char cs_id; /* connection # associated with this state */
u_char cs_filler;
! struct ip cs_ip;/* ip/tcp hdr from most recent packet */
};
/*
Bruce
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
In message <[EMAIL PROTECTED]>, Gary Jennejohn writes: >Yes, but it was perfectly legal to put the structure on the stack >_before_ MLEN was doubled. Just because it worked doesn't mean that it was correct. We need to be frugal about the kernel stack, for a lot of reasons, that's just the way it is, and as far as I know it is the way it will continue to be. Get used to it. -- Poul-Henning Kamp FreeBSD coreteam member [EMAIL PROTECTED] "Real hackers run -current on their laptop." FreeBSD -- It will take a long time before progress goes too far! To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
Alfred Perlstein writes: >* Hellmuth Michaelis <[EMAIL PROTECTED]> [000403 02:12] wrote: >> Please don't misunderstand. I can fully accept accecpt and acknowledge what >> you write (i've converted the piece of code in question to malloc'ing its >> data already), i'm just a bit concerned because its so perfectly legal and >> common to allocate a struct on the stack that i fear just that i and >> probably other don't think about it if it were not made absolutely clear >> and possibly enforced somehow to never do this in kernel land. > >When in doubt, ask. Design and Implementation clearly explains the >kernel stack program as well as some other OS texts afaik. > >Just because it's legal C doesn't mean it's allowed, it's perfectly legal >to do a lot of things in a usermode program that you can't do in a >kernel routine, smashing the kernel stack is one of these things. :) > Yes, but it was perfectly legal to put the structure on the stack _before_ MLEN was doubled. If this is the attitude which now obtains, then there's a boatload of code in the kernel which is incorrect because structs are allocated on the stack without knowing how big they are at the *time of compilation*. This isn't a question of "was the routine sppp_params sloppily coded ?" (which I think Joerg would disagree with, since he wrote it). I still think that we should nuke csu_hdr from struct slcompress. It's not used in any code in our tree. I did a make buildowrld and a new kernel without csu_hdr and there were no errors. The change even made the kernel BSS 25KB smaller. --- Gary Jennejohn / [EMAIL PROTECTED] [EMAIL PROTECTED] [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
* Hellmuth Michaelis <[EMAIL PROTECTED]> [000403 02:12] wrote:
> >From the keyboard of Bruce Evans:
>
> > It's just a bug to allocate big structs on the kernel stack.
>
> Please specify "big"! :-)
have a look at src/sys/nfs/nfs_vnops.c:
line ~2787:
#ifndef NFS_COMMITBVECSIZ
#define NFS_COMMITBVECSIZ 20
#endif
struct buf *bvec_on_stack[NFS_COMMITBVECSIZ];
int bvecsize = 0, bveccount;
I guess 80 bytes is pushing it, some routines are worse, but 2k
is totally out of line.
If you're worried about such things happening then you can use
the pre-processor to catch things that may make your structures
too large.
> I wonder how too "big" can be detected. The code in question is perfectly
> valid syntactically and semantically correct C-code.
There's code that #error (maybe #warning?) when it can detect that the
size of a struct has been unreasonably grown, it's done via the
pre-processor.
>
> If a piece of code being considered buggy depends on the size of some
> externally defined "thing" (like the kernel stack size in this case)
> something - IMHO - is not in a good condition.
>
> Perhaps a sentence like "It's just a bug to allocate structs on the
> kernel stack" (note the missing "big") has to be put in style. Or a
> macro like (this is just a cheap shoot in the dark)
>
> void
> kernel_subr(void)
> {
> ALLOCATE_STRUCT_ON_STACK(struct bigstruct bigstr);
>
>
> could be created to check for potentially stack overflow at compile
> time and/or runtime and people are forced in style(9) to use it.
noo :)
> > 512 bytes is probably too big for i386's now. The effective kernel stack
> > size was shrunk by about 2K by the sigset_t changes, so there is only
> > about 5K of kernel stack.
>
> Then isn't it time to make the kernel stack larger ?
>
> Please don't misunderstand. I can fully accept accecpt and acknowledge what
> you write (i've converted the piece of code in question to malloc'ing its
> data already), i'm just a bit concerned because its so perfectly legal and
> common to allocate a struct on the stack that i fear just that i and
> probably other don't think about it if it were not made absolutely clear
> and possibly enforced somehow to never do this in kernel land.
When in doubt, ask. Design and Implementation clearly explains the
kernel stack program as well as some other OS texts afaik.
Just because it's legal C doesn't mean it's allowed, it's perfectly legal
to do a lot of things in a usermode program that you can't do in a
kernel routine, smashing the kernel stack is one of these things. :)
--
-Alfred Perlstein - [[EMAIL PROTECTED]|[EMAIL PROTECTED]]
"I have the heart of a child; I keep it in a jar on my desk."
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
>From the keyboard of Bruce Evans:
> It's just a bug to allocate big structs on the kernel stack.
Please specify "big"! :-)
I wonder how too "big" can be detected. The code in question is perfectly
valid syntactically and semantically correct C-code.
If a piece of code being considered buggy depends on the size of some
externally defined "thing" (like the kernel stack size in this case)
something - IMHO - is not in a good condition.
Perhaps a sentence like "It's just a bug to allocate structs on the
kernel stack" (note the missing "big") has to be put in style. Or a
macro like (this is just a cheap shoot in the dark)
void
kernel_subr(void)
{
ALLOCATE_STRUCT_ON_STACK(struct bigstruct bigstr);
could be created to check for potentially stack overflow at compile
time and/or runtime and people are forced in style(9) to use it.
> 512 bytes is probably too big for i386's now. The effective kernel stack
> size was shrunk by about 2K by the sigset_t changes, so there is only
> about 5K of kernel stack.
Then isn't it time to make the kernel stack larger ?
Please don't misunderstand. I can fully accept accecpt and acknowledge what
you write (i've converted the piece of code in question to malloc'ing its
data already), i'm just a bit concerned because its so perfectly legal and
common to allocate a struct on the stack that i fear just that i and
probably other don't think about it if it were not made absolutely clear
and possibly enforced somehow to never do this in kernel land.
hellmuth
--
Hellmuth MichaelisTel +49 40 55 97 47-70
HCS Hanseatischer Computerservice GmbHFax +49 40 55 97 47-77
Oldesloer Strasse 97-99 Mail hm [at] hcs.de
D-22457 Hamburg WWW http://www.hcs.de
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
Bruce Evans writes: >On Sun, 2 Apr 2000, Gary Jennejohn wrote: >> Moving the struct spppreq into global address space solves the problem, >> but that makes the kernel BSS somewhat larger. Redefining MAX_HDR to be >> 128 also fixes the problem, even with the struct spppreq on the stack. > >Big structs need to be malloced. > Yes, but how does one know that a struct is too big ? Before the increase in MLEN strucct sppp was not too big. >I think removing the unused bloat in `struct cstate' is the correct fix >for this particular allocation. > I think we should nuke csu_hdr since it's not used anywhere. Is that what you really mean ? --- Gary Jennejohn / [EMAIL PROTECTED] [EMAIL PROTECTED] [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
> I wonder how wise it was to change MLEN without more testing. But hey, > this is -current, that's what it's there for. I've been running with MLEN set to 256 bytes for more than a year for reasons unrelated to this commit, and haven't seen any problems at all. (Of course, I don't use sppp..) It's unclear how this could have been caught by more testing unless the tester was using sppp. louie To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
On Sun, 2 Apr 2000, Gary Jennejohn wrote: > struct slcompress is now in struct sppp, which is passed by ispppcontrol > as part of an ioctl call. Eventually the kernel lands in sppp_params, > which does a copyin to a struct spppreq (which contains struct sppp) on > the kernel stack. Because struct sppp is 2KB larger as a result of the > change in MLEN the copyin overruns the kernel stack which immediately > results in a crash - no trace output, no ddb, zilch. It's just a bug to allocate big structs on the kernel stack. Almost all structs are too big for utility routines. Top levels of syscall routines and a few other routines that are known not to be called from deep in the kernel stack can use moderately big structs. 512 bytes is probably too big for i386's now. The effective kernel stack size was shrunk by about 2K by the sigset_t changes, so there is only about 5K of kernel stack. > Interesting is that the crash only happens on a Pentium (tested with > a II and III). On a K6 it doesn't happen. AFAICT it's not related to > using the FPU for copyin/copyout since I turned that functionality > off using npx0 flags and the crash still happened. The behaviour is unpredictable. Severe cases will be detected as a double fault if you're lucky. The sigset_t changes gave an extra 2K of signal variables to clobber before there is a double fault :-). > Moving the struct spppreq into global address space solves the problem, > but that makes the kernel BSS somewhat larger. Redefining MAX_HDR to be > 128 also fixes the problem, even with the struct spppreq on the stack. Big structs need to be malloced. I think removing the unused bloat in `struct cstate' is the correct fix for this particular allocation. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: MLEN and crashes
* Gary Jennejohn <[EMAIL PROTECTED]> [000402 01:43] wrote: > This is a HEADS UP. > > The recent increase in MLEN from 128 to 256 bytes led to very surprising > problems with the latest, so called developers', version of isdn4bsd. > > The new version uses slcompress by default. The change in MLEN makes > struct slcompress 2KB larger than it used to be. BTW the entry csu_hdr > in struct cstate, which has size MLEN, is not used anywhere in the kernel > that I could find. csu_hdr is what leads to the increase in the size of > struct slcompress. There's a comment in slcompress.h which states that > MAX_HDR should really be defined as 128 and not MLEN. Maybe this should > be taken to heart and MAX_HDR redefined as 128 and not MLEN. > > But I digress. > > struct slcompress is now in struct sppp, which is passed by ispppcontrol > as part of an ioctl call. Eventually the kernel lands in sppp_params, > which does a copyin to a struct spppreq (which contains struct sppp) on > the kernel stack. Because struct sppp is 2KB larger as a result of the > change in MLEN the copyin overruns the kernel stack which immediately > results in a crash - no trace output, no ddb, zilch. > Interesting is that the crash only happens on a Pentium (tested with > a II and III). On a K6 it doesn't happen. AFAICT it's not related to > using the FPU for copyin/copyout since I turned that functionality > off using npx0 flags and the crash still happened. > > Moving the struct spppreq into global address space solves the problem, > but that makes the kernel BSS somewhat larger. Redefining MAX_HDR to be > 128 also fixes the problem, even with the struct spppreq on the stack. > > If those of you using slcompress start seeing problems then they may well > be due to the increase in MLEN. > > I wonder how wise it was to change MLEN without more testing. But hey, > this is -current, that's what it's there for. > > Anyway, I think MAX_HDR should be hardwired to 128 in slcompress. Any > comments ? > Why not just malloc the structure instread of using a stack variable? Various other parts of the kernel do so to get around this problem, since we're heading up SMP now it _not_ the time to start using global variables. -- -Alfred Perlstein - [[EMAIL PROTECTED]|[EMAIL PROTECTED]] "I have the heart of a child; I keep it in a jar on my desk." To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
MLEN and crashes
This is a HEADS UP. The recent increase in MLEN from 128 to 256 bytes led to very surprising problems with the latest, so called developers', version of isdn4bsd. The new version uses slcompress by default. The change in MLEN makes struct slcompress 2KB larger than it used to be. BTW the entry csu_hdr in struct cstate, which has size MLEN, is not used anywhere in the kernel that I could find. csu_hdr is what leads to the increase in the size of struct slcompress. There's a comment in slcompress.h which states that MAX_HDR should really be defined as 128 and not MLEN. Maybe this should be taken to heart and MAX_HDR redefined as 128 and not MLEN. But I digress. struct slcompress is now in struct sppp, which is passed by ispppcontrol as part of an ioctl call. Eventually the kernel lands in sppp_params, which does a copyin to a struct spppreq (which contains struct sppp) on the kernel stack. Because struct sppp is 2KB larger as a result of the change in MLEN the copyin overruns the kernel stack which immediately results in a crash - no trace output, no ddb, zilch. Interesting is that the crash only happens on a Pentium (tested with a II and III). On a K6 it doesn't happen. AFAICT it's not related to using the FPU for copyin/copyout since I turned that functionality off using npx0 flags and the crash still happened. Moving the struct spppreq into global address space solves the problem, but that makes the kernel BSS somewhat larger. Redefining MAX_HDR to be 128 also fixes the problem, even with the struct spppreq on the stack. If those of you using slcompress start seeing problems then they may well be due to the increase in MLEN. I wonder how wise it was to change MLEN without more testing. But hey, this is -current, that's what it's there for. Anyway, I think MAX_HDR should be hardwired to 128 in slcompress. Any comments ? Gary Jennejohn / [EMAIL PROTECTED] [EMAIL PROTECTED] [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
