Celelibi
2016-Jan-22 11:59 UTC
[syslinux] [PATCH 2/2] core: Fix stack overflow when reloading config
2016-01-22 3:37 UTC+01:00, H. Peter Anvin <hpa at zytor.com>:> On 01/21/16 18:33, Celelibi wrote: >> >> BTW, this code is due to Matt Flemming with commit 3a316db1 (later >> modified). The commit log say this: >> ldlinux: Loading a config file should cause re-initialisation >> >> There are a number of initialisation steps that need to be performed >> *every* time a config file is loaded. Reload ldlinux.c32 so that we >> can re-initialise the environment whenever a new config file is >> loaded. This involves unloading all the modules that have been loaded >> since ldlinux.c32. Luckily the list of loaded modules is sorted by >> load order, which means it's trivial to "pop" them from the front of >> the list. >> > > Yes, it is the easy way to initialize. Part of me wonders if we should > keep a copy of the data section and just wipe it out. > > -hpaI see 3 ways of handling this. 1) Have some specific code in ldlinux.c32 that handles reinitialization. 2) Have some specific cache for the COM32 modules and load them only once for the lifetime of the whole boot loader. 3) Put a file system cache that would also benefit to other files. I would tend to prefer the third way, but I don't know how much work it would be or if it would integrate well in the current design. Celelibi
H. Peter Anvin
2016-Jan-25 11:28 UTC
[syslinux] [PATCH 2/2] core: Fix stack overflow when reloading config
On January 22, 2016 3:59:09 AM PST, Celelibi <celelibi at gmail.com> wrote:>2016-01-22 3:37 UTC+01:00, H. Peter Anvin <hpa at zytor.com>: >> On 01/21/16 18:33, Celelibi wrote: >>> >>> BTW, this code is due to Matt Flemming with commit 3a316db1 (later >>> modified). The commit log say this: >>> ldlinux: Loading a config file should cause re-initialisation >>> >>> There are a number of initialisation steps that need to be >performed >>> *every* time a config file is loaded. Reload ldlinux.c32 so that >we >>> can re-initialise the environment whenever a new config file is >>> loaded. This involves unloading all the modules that have been >loaded >>> since ldlinux.c32. Luckily the list of loaded modules is sorted >by >>> load order, which means it's trivial to "pop" them from the >front of >>> the list. >>> >> >> Yes, it is the easy way to initialize. Part of me wonders if we >should >> keep a copy of the data section and just wipe it out. >> >> -hpa > >I see 3 ways of handling this. >1) Have some specific code in ldlinux.c32 that handles >reinitialization. >2) Have some specific cache for the COM32 modules and load them only >once for the lifetime of the whole boot loader. >3) Put a file system cache that would also benefit to other files. > >I would tend to prefer the third way, but I don't know how much work >it would be or if it would integrate well in the current design. > > >Celelibi#2 is really easy; we just need to keep a copy of the unmodified data section. The only risk with this approach is running out of memory, but I think that risk is minor. -- Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
Celelibi
2016-Jan-25 18:51 UTC
[syslinux] [PATCH 2/2] core: Fix stack overflow when reloading config
2016-01-25 12:28 UTC+01:00, H. Peter Anvin <hpa at zytor.com>:> On January 22, 2016 3:59:09 AM PST, Celelibi <celelibi at gmail.com> wrote: >>2016-01-22 3:37 UTC+01:00, H. Peter Anvin <hpa at zytor.com>: >>> On 01/21/16 18:33, Celelibi wrote: >>>> >>>> BTW, this code is due to Matt Flemming with commit 3a316db1 (later >>>> modified). The commit log say this: >>>> ldlinux: Loading a config file should cause re-initialisation >>>> >>>> There are a number of initialisation steps that need to be >>performed >>>> *every* time a config file is loaded. Reload ldlinux.c32 so that >>we >>>> can re-initialise the environment whenever a new config file is >>>> loaded. This involves unloading all the modules that have been >>loaded >>>> since ldlinux.c32. Luckily the list of loaded modules is sorted >>by >>>> load order, which means it's trivial to "pop" them from the >>front of >>>> the list. >>>> >>> >>> Yes, it is the easy way to initialize. Part of me wonders if we >>should >>> keep a copy of the data section and just wipe it out. >>> >>> -hpa >> >>I see 3 ways of handling this. >>1) Have some specific code in ldlinux.c32 that handles >>reinitialization. >>2) Have some specific cache for the COM32 modules and load them only >>once for the lifetime of the whole boot loader. >>3) Put a file system cache that would also benefit to other files. >> >>I would tend to prefer the third way, but I don't know how much work >>it would be or if it would integrate well in the current design. >> >> >>Celelibi > > #2 is really easy; we just need to keep a copy of the unmodified data > section. The only risk with this approach is running out of memory, but I > think that risk is minor.Do you suggest to do this only for ldlinux.c32? If we keep a copy only of the data section, we will only be able to use it when we know we reload the module. So it will only apply to ldlinux.c32. While most modules could benefit from this as the new config file will probably reload the same menu and libraries. Celelibi
Ady
2016-Jan-25 22:21 UTC
[syslinux] [PATCH 2/2] core: Fix stack overflow when reloading config
> >I see 3 ways of handling this. > >1) Have some specific code in ldlinux.c32 that handles > >reinitialization. > >2) Have some specific cache for the COM32 modules and load them only > >once for the lifetime of the whole boot loader. > >3) Put a file system cache that would also benefit to other files. > > > >I would tend to prefer the third way, but I don't know how much work > >it would be or if it would integrate well in the current design. > > > > > >Celelibi > > #2 is really easy; we just need to keep a copy of the unmodified > data section. The only risk with this approach is running out of > memory, but I think that risk is minor. --Please forgive my lack of technical knowledge about the code itself. Regarding "...running out of memory... the risk is minor." This "wishful thinking" reminds me of a different case we had; although, code-wise, it was not a similar problem (I would assume). A patch from Shao, "fs: Fix searchdir resource leak", committed 2012Nov, _ for 5.xx commit 57acc34bdf83fc5ea08dbf44b74a5dd2c1131187 _ for 4.xx commit 37971728a5fc40b1c90512e79e47333d98ec8851 reads: *** This is a significant rewrite of the generic lookup logic inside core/fs/fs.c's searchdir function. Previously, there was a memory leak if a path involved multiple directories. After a sufficiently large number of invocations, this could be observed. *** The code, before that patch, used to assume that just "a few" files were involved. A c32 module that has to deal with many more files showed the leak, and the crash. (BTW, I am still waiting - after more than 3 years - and hoping for that useful c32 module to be part of official Syslinux.) Additionally, the amount of available resources (including memory, among others) in some cases is not _that_ high, and we frequently forget they exist. Cases such as the DOS-based installer comes to mind, and the length of the file names for library modules (e.g. "libutil_com.c32") is another one. It was _that_ easy to forget that FAT and ISO9660 do not support such file names (thus, a library module shall not use unsupported file names), and it is easy for anyone to assume that "his own" use-case is broad-enough that it covers anyone's / everyone's else. It is also not a coincidence that we already have seen leaks related to directives such as CONFIG and INCLUDE in the past. I hope we are not introducing more problems while we are trying to solve one. Regards, Ady.
Apparently Analagous Threads
- [PATCH 2/2] core: Fix stack overflow when reloading config
- [PATCH 2/2] core: Fix stack overflow when reloading config
- [PATCH 2/2] core: Fix stack overflow when reloading config
- [PATCH 2/2] core: Fix stack overflow when reloading config
- [PATCH 2/2] core: Fix stack overflow when reloading config