Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow
Hi,
On Mon, Sep 26, 2011 at 11:46 PM, Gilles Chanteperdrix
wrote:
> On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote:
>> Hi,
>>
>> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus wrote:
>>> On Mon, Sep 19, 2011 at 9:25 AM, wrote:
> From: xenomai-help-boun...@gna.org [mailto:xenomai-help-boun...@gna.org]
> On Behalf Of Philippe Gerum
> Sent: Sunday, September 18, 2011 5:37 PM
> ...
> Actually, we used to follow strictly the pSOS convention for this until
> 2.4.x, at which point we moved to name strings precisely because
> non-null terminated char[4] arrays would break the registry, the way you
> described. This is one of the rare situations where mimicking a useless
> limitation of the original API may be challenged by usability concerns
> in the new environment, and usability won in that case. The problem
> mostly comes from the fact that char[4] is automatically convertible to
> const char *, so we have no warning/error leading us to check the
> potentially problematic call sites.
>
> The concern about moving back to char[4] arrays - null-terminated if
> shorter - is for people who currently assign strings longer than 4
> characters to name their objects. What could be done, is providing a
> build switch to select the accepted input, like
> --disable-psos-long-names to turn on char[4] interpretation.
While I would prefer the switch --enable-psos-long-names, this sounds okay
to me, the more so as it is more than people who use the pSOS skin without
obeying pSOS rules deserve.
--
>> [..]
>>>
>>> Another option would be to introduce a run-time switch.
>>> The default behavior could be that long names are supported and if the
>>> "enable_short_names()" function is called, all names will the cut at 4
>>> characters.
>>> The advantage of this dynamic switch is that different applications
>>> using the same library can use the mode they prefer.
>>
>> I would also be in favor of a runtime setting, rather than a
>> compile-time one. One cannot assume that all PSOS applications on a
>> system follow the same rules, or that there cannot be a mix of
>> 'legacy' PSOS applications and 'xenomai-style' ones. According to me,
>> a library should support all these uses.
>
> Hi,
>
> here is a patch which truncates identifiers to four characters and
> terminates them by a '\0' character, in order to avoid the issues at
> the origin of this thread. The patch also adds a variable
> "psos_long_names", 0 by default, which may be set to a non-zero value
> to enable the old behaviour (assume the identifier strings may be
> longer than 4 characters and are already null terminated).
>
> Could someone with the issue test it?
It seems this slipped through the cracks, my apologies. We already
implemented this ourselves similarly, we didn't actually test your
patch so far.
Now that we're trying out xenomai-forge, I ported this patch and tested it.
There are a few problems with it (also relevant to cobalt xenomai)
* a bug in __psos_maybe_short_name so that only 3 characters are
retained (see below)
* the call to __psos_maybe_short_name also needs to be done in the
_ident functions
* although we don't use it, I think the pt_create and pt_ident
functions also need to be adapted
Moreover, I wonder why you have made psos_long_names an exported
global variable. Sharing variables between two different pieces of
software is not very clean. I think it would be nicer with a get/set
function.
> diff --git a/include/psos+/psos.h b/include/psos+/psos.h
> index 32281bc..4a3921a 100644
> --- a/include/psos+/psos.h
> +++ b/include/psos+/psos.h
> @@ -197,6 +197,8 @@ u_long t_restart(u_long tid,
>
> #include
>
> +extern unsigned psos_long_names;
> +
> #endif /* __KERNEL__ || __XENO_SIM__ */
>
> /*
> diff --git a/src/skins/psos+/init.c b/src/skins/psos+/init.c
> index 978a4f1..e53967b 100644
> --- a/src/skins/psos+/init.c
> +++ b/src/skins/psos+/init.c
> @@ -26,6 +26,8 @@ int __psos_muxid = -1;
>
> xnsysinfo_t __psos_sysinfo;
>
> +unsigned psos_long_names;
> +
> static __attribute__ ((constructor))
> void __init_xeno_interface(void)
> {
> @@ -68,3 +70,14 @@ void k_fatal(u_long err_code, u_long flags)
> {
> exit(1);
> }
> +
> +const char *__psos_maybe_short_name(char shrt[5], const char *lng)
> +{
> + if (psos_long_names)
> + return lng;
> +
> + strncpy(shrt, lng, sizeof(shrt) - 1);
Because shrt is passed as parameter, it is passed as pointer and not
as array. This means there is no size information, and thus
sizeof(shrt) equals the size of a pointer (4 on our 32-bit platform).
As a result, only ABC of a name ABCD are copied.
You'll need to explicitly do:
strncpy(shrt, lng, 5 - 1);
> + shrt[4] = '\0';
> +
> + return (const char *)shrt;
> +}
> diff --git a/src/skins/psos+/queue.c b/src/skins/psos+/queue.c
> index c54f966..c8c1933 100644
> --- a/src/skins/psos+/queue.c
> +++ b/src/
Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow
On 10/12/2011 04:03 PM, Thomas De Schampheleire wrote: > Hi, > > On Mon, Sep 26, 2011 at 11:46 PM, Gilles Chanteperdrix > wrote: >> On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote: >>> Hi, >>> >>> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus wrote: On Mon, Sep 19, 2011 at 9:25 AM, wrote: >> From: xenomai-help-boun...@gna.org [mailto:xenomai-help-boun...@gna.org] >> On Behalf Of Philippe Gerum >> Sent: Sunday, September 18, 2011 5:37 PM >> ... >> Actually, we used to follow strictly the pSOS convention for this until >> 2.4.x, at which point we moved to name strings precisely because >> non-null terminated char[4] arrays would break the registry, the way you >> described. This is one of the rare situations where mimicking a useless >> limitation of the original API may be challenged by usability concerns >> in the new environment, and usability won in that case. The problem >> mostly comes from the fact that char[4] is automatically convertible to >> const char *, so we have no warning/error leading us to check the >> potentially problematic call sites. >> >> The concern about moving back to char[4] arrays - null-terminated if >> shorter - is for people who currently assign strings longer than 4 >> characters to name their objects. What could be done, is providing a >> build switch to select the accepted input, like >> --disable-psos-long-names to turn on char[4] interpretation. > > While I would prefer the switch --enable-psos-long-names, this sounds > okay to me, the more so as it is more than people who use the pSOS skin > without obeying pSOS rules deserve. > -- >>> [..] > Another option would be to introduce a run-time switch. The default behavior could be that long names are supported and if the "enable_short_names()" function is called, all names will the cut at 4 characters. The advantage of this dynamic switch is that different applications using the same library can use the mode they prefer. >>> >>> I would also be in favor of a runtime setting, rather than a >>> compile-time one. One cannot assume that all PSOS applications on a >>> system follow the same rules, or that there cannot be a mix of >>> 'legacy' PSOS applications and 'xenomai-style' ones. According to me, >>> a library should support all these uses. >> >> Hi, >> >> here is a patch which truncates identifiers to four characters and >> terminates them by a '\0' character, in order to avoid the issues at >> the origin of this thread. The patch also adds a variable >> "psos_long_names", 0 by default, which may be set to a non-zero value >> to enable the old behaviour (assume the identifier strings may be >> longer than 4 characters and are already null terminated). >> >> Could someone with the issue test it? > > It seems this slipped through the cracks, my apologies. We already > implemented this ourselves similarly, we didn't actually test your > patch so far. > > Now that we're trying out xenomai-forge, I ported this patch and tested it. > There are a few problems with it (also relevant to cobalt xenomai) > > * a bug in __psos_maybe_short_name so that only 3 characters are > retained (see below) > * the call to __psos_maybe_short_name also needs to be done in the > _ident functions > * although we don't use it, I think the pt_create and pt_ident > functions also need to be adapted > > Moreover, I wonder why you have made psos_long_names an exported > global variable. Sharing variables between two different pieces of > software is not very clean. I think it would be nicer with a get/set > function. Thanks for the review. The exported global variable is the simplest possible and sufficient implementation of this feature. You risk having a hard time convincing us otherwise. -- Gilles. ___ Xenomai-help mailing list Xenomai-help@gna.org https://mail.gna.org/listinfo/xenomai-help
Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow
On Wed, 2011-10-12 at 16:22 +0200, Gilles Chanteperdrix wrote: > On 10/12/2011 04:03 PM, Thomas De Schampheleire wrote: > > Hi, > > > > On Mon, Sep 26, 2011 at 11:46 PM, Gilles Chanteperdrix > > wrote: > >> On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote: > >>> Hi, > >>> > >>> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus > >>> wrote: > On Mon, Sep 19, 2011 at 9:25 AM, > wrote: > >> From: xenomai-help-boun...@gna.org > >> [mailto:xenomai-help-boun...@gna.org] > >> On Behalf Of Philippe Gerum > >> Sent: Sunday, September 18, 2011 5:37 PM > >> ... > >> Actually, we used to follow strictly the pSOS convention for this until > >> 2.4.x, at which point we moved to name strings precisely because > >> non-null terminated char[4] arrays would break the registry, the way > >> you > >> described. This is one of the rare situations where mimicking a useless > >> limitation of the original API may be challenged by usability concerns > >> in the new environment, and usability won in that case. The problem > >> mostly comes from the fact that char[4] is automatically convertible to > >> const char *, so we have no warning/error leading us to check the > >> potentially problematic call sites. > >> > >> The concern about moving back to char[4] arrays - null-terminated if > >> shorter - is for people who currently assign strings longer than 4 > >> characters to name their objects. What could be done, is providing a > >> build switch to select the accepted input, like > >> --disable-psos-long-names to turn on char[4] interpretation. > > > > While I would prefer the switch --enable-psos-long-names, this sounds > > okay to me, the more so as it is more than people who use the pSOS skin > > without obeying pSOS rules deserve. > > -- > >>> [..] > > > > Another option would be to introduce a run-time switch. > The default behavior could be that long names are supported and if the > "enable_short_names()" function is called, all names will the cut at 4 > characters. > The advantage of this dynamic switch is that different applications > using the same library can use the mode they prefer. > >>> > >>> I would also be in favor of a runtime setting, rather than a > >>> compile-time one. One cannot assume that all PSOS applications on a > >>> system follow the same rules, or that there cannot be a mix of > >>> 'legacy' PSOS applications and 'xenomai-style' ones. According to me, > >>> a library should support all these uses. > >> > >> Hi, > >> > >> here is a patch which truncates identifiers to four characters and > >> terminates them by a '\0' character, in order to avoid the issues at > >> the origin of this thread. The patch also adds a variable > >> "psos_long_names", 0 by default, which may be set to a non-zero value > >> to enable the old behaviour (assume the identifier strings may be > >> longer than 4 characters and are already null terminated). > >> > >> Could someone with the issue test it? > > > > It seems this slipped through the cracks, my apologies. We already > > implemented this ourselves similarly, we didn't actually test your > > patch so far. > > > > Now that we're trying out xenomai-forge, I ported this patch and tested it. > > There are a few problems with it (also relevant to cobalt xenomai) > > > > * a bug in __psos_maybe_short_name so that only 3 characters are > > retained (see below) > > * the call to __psos_maybe_short_name also needs to be done in the > > _ident functions > > * although we don't use it, I think the pt_create and pt_ident > > functions also need to be adapted > > > > Moreover, I wonder why you have made psos_long_names an exported > > global variable. Sharing variables between two different pieces of > > software is not very clean. I think it would be nicer with a get/set > > function. > > Thanks for the review. The exported global variable is the simplest > possible and sufficient implementation of this feature. You risk having > a hard time convincing us otherwise. Ack. > > -- > Gilles. > > ___ > Xenomai-help mailing list > Xenomai-help@gna.org > https://mail.gna.org/listinfo/xenomai-help -- Philippe. ___ Xenomai-help mailing list Xenomai-help@gna.org https://mail.gna.org/listinfo/xenomai-help
Re: [Xenomai-help] Command code working with comedi not working with analogy
El 11 de octubre de 2011 19:12, Alexis Berlemont escribió:
> Hi,
>
> Sorry for answering late.
>
Well, thanks for answering ;)
>
> I took some time to compare both versions of code (comedi and
> analogy). I did not find anything interesting in mite.c. I was about
> to ask you to increase verbosity (debug + a specific patch) when I got
> a glimpse on the allocation of the asynchronous buffer on the comedi
> side.
>
> The methods are not the same at that level:
> - comedi: n * dma_alloc_coherent + a vmap at the end
> - analogy: a big vmalloc + n * page_to_phys(vmalloc_to_page(vaddr)
>
> I don't understand the reason why the analogy way is not correct, I
> did not have time to check yet. If anyone has any idea, please help
> me.
>
> However, the NI mite expects 32 bit addresses... (because of
> cpu_to_le32 in the mite driver).
>
> If I remember well, you are using a 64-bit architecture. Is it
> correct? There may be a bug here; analogy might give to the mite
> physical addresses which are not below 2^32.
>
That's right, I'm on a 64bit machine. This sounds very reasonable to me.
> So, could you:
> - with comedi, add some printk("%lx") in mite_buf_change so as to dump
> the pages physical addresses
> - with analogy, do the same thing in a4l_mite_buf_change.
>
Ok, I'v only performed the analogy tests so far, and here are the results:
[ 172.627662] Analogy: sizeof(dma_addr_t) = 8
[ 172.627663] Analogy: ring->descriptors_dma_addr = c740d000
[ 172.627665] Analogy: cpu_to_le32(ring->descriptors_dma_addr) = c740d000
[ 172.627666] Analogy: buf->pg_list[0] = 19d806000
[ 172.627667] Analogy: buf->pg_list[1] = 1a9095000
[ 172.627668] Analogy: buf->pg_list[2] = 19d552000
[ 172.627669] Analogy: buf->pg_list[3] = 19eb93000
[ 172.627671] Analogy: buf->pg_list[4] = 1a91fc000
[ 172.627672] Analogy: buf->pg_list[5] = 1a9203000
[ 172.627673] Analogy: buf->pg_list[6] = 19d9c1000
[ 172.627674] Analogy: buf->pg_list[7] = 19ebff000
[ 172.627675] Analogy: buf->pg_list[8] = 19eacb000
[ 172.627676] Analogy: buf->pg_list[9] = 1a905d000
[ 172.627677] Analogy: buf->pg_list[10] = 1a905
[ 172.627678] Analogy: buf->pg_list[11] = 1a9174000
[ 172.627679] Analogy: buf->pg_list[12] = 1aa791000
[ 172.627680] Analogy: buf->pg_list[13] = 1aa635000
[ 172.627682] Analogy: buf->pg_list[14] = 1aa7fb000
[ 172.627683] Analogy: buf->pg_list[15] = 19dae5000
These are printed right after the call to "pci_alloc_consistent()" and in
the subsequent "for" loop. So if I understand it right,
"ring->descriptors_dma_addr" represents the base physical address that DMA
transfers will access? And then, "buf->pg_list" are virtual addresses or
virtual page indexes?
I'll check those values in comedi and let you know.
Thanks a lot for your effort!
Fernando
___
Xenomai-help mailing list
Xenomai-help@gna.org
https://mail.gna.org/listinfo/xenomai-help
Re: [Xenomai-help] Xenomai-forge: SEGFAULT is pSOS skin
On Tue, Oct 4, 2011 at 5:41 PM, Philippe Gerum wrote:
> On Mon, 2011-09-26 at 22:01 +0200, Ronny Meeus wrote:
>
>
>
>> Next to this I also adapted the task priority automatically using
>> following algorithm:
>> static int check_task_priority(u_long *psos_prio)
>> {
>> if (*psos_prio < 1 || *psos_prio > 255) /* In theory. */
>> return ERR_PRIOR;
>> /* Do not change priorities <=10 and >= 240.
>> * Priorities in between are divided by 4 */
>> if (*psos_prio > 240)
>> *psos_prio = 70 + *psos_prio - 240;
>> else if (*psos_prio > 10)
>> *psos_prio = 11 + ((*psos_prio-10)/4);
>>
>> if ((int)(*psos_prio) >= threadobj_max_prio - 1) /* In practice. */
>> panic("current implementation restricts pSOS "
>> "priority levels to range [1..%d]",
>> threadobj_max_prio - 2);
>>
>> return SUCCESS;
>> }
>>
>> It also works well for our application.
>> Please share your thoughts.
>
> Since we cannot generalize the priority mapping rules, a better way may
> be to allow your own code to be called by the pSOS emulator when such
> mapping is required. So I have committed a tentative solution, defining
> psos_task_normalize_priority() as a weak function, which receives the
> pSOS priority, and should return the POSIX one.
>
> A default implementation is provided by the emulator which does a
> trivial 1:1 mapping.
>
> --
> Philippe.
>
>
>
Philippe
as Thomas indicated in his mail today (see PSOS skin: mismatch in
function signatures cause buffer overflow), we synced with the forge
repo. We used this psos_task_normalize_priority function to remap the
priority for our application and it works well.
Thanks.
Ronny
___
Xenomai-help mailing list
Xenomai-help@gna.org
https://mail.gna.org/listinfo/xenomai-help
Re: [Xenomai-help] Xenomai-forge: SEGFAULT is pSOS skin
On Wed, 2011-10-12 at 19:16 +0200, Ronny Meeus wrote:
> On Tue, Oct 4, 2011 at 5:41 PM, Philippe Gerum wrote:
> > On Mon, 2011-09-26 at 22:01 +0200, Ronny Meeus wrote:
> >
> >
> >
> >> Next to this I also adapted the task priority automatically using
> >> following algorithm:
> >> static int check_task_priority(u_long *psos_prio)
> >> {
> >> if (*psos_prio < 1 || *psos_prio > 255) /* In theory. */
> >> return ERR_PRIOR;
> >> /* Do not change priorities <=10 and >= 240.
> >> * Priorities in between are divided by 4 */
> >> if (*psos_prio > 240)
> >> *psos_prio = 70 + *psos_prio - 240;
> >> else if (*psos_prio > 10)
> >> *psos_prio = 11 + ((*psos_prio-10)/4);
> >>
> >> if ((int)(*psos_prio) >= threadobj_max_prio - 1) /* In practice. */
> >> panic("current implementation restricts pSOS "
> >> "priority levels to range [1..%d]",
> >> threadobj_max_prio - 2);
> >>
> >> return SUCCESS;
> >> }
> >>
> >> It also works well for our application.
> >> Please share your thoughts.
> >
> > Since we cannot generalize the priority mapping rules, a better way may
> > be to allow your own code to be called by the pSOS emulator when such
> > mapping is required. So I have committed a tentative solution, defining
> > psos_task_normalize_priority() as a weak function, which receives the
> > pSOS priority, and should return the POSIX one.
> >
> > A default implementation is provided by the emulator which does a
> > trivial 1:1 mapping.
> >
> > --
> > Philippe.
> >
> >
> >
>
> Philippe
>
> as Thomas indicated in his mail today (see PSOS skin: mismatch in
> function signatures cause buffer overflow), we synced with the forge
> repo. We used this psos_task_normalize_priority function to remap the
> priority for our application and it works well.
> Thanks.
Ok, so this interface is now official. Thanks.
>
> Ronny
--
Philippe.
___
Xenomai-help mailing list
Xenomai-help@gna.org
https://mail.gna.org/listinfo/xenomai-help
Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow
On Wed, Oct 12, 2011 at 5:56 PM, Philippe Gerum wrote: > On Wed, 2011-10-12 at 16:22 +0200, Gilles Chanteperdrix wrote: >> On 10/12/2011 04:03 PM, Thomas De Schampheleire wrote: >> > Hi, >> > >> > On Mon, Sep 26, 2011 at 11:46 PM, Gilles Chanteperdrix >> > wrote: >> >> On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote: >> >>> Hi, >> >>> >> >>> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus >> >>> wrote: >> On Mon, Sep 19, 2011 at 9:25 AM, >> wrote: >> >> From: xenomai-help-boun...@gna.org >> >> [mailto:xenomai-help-boun...@gna.org] >> >> On Behalf Of Philippe Gerum >> >> Sent: Sunday, September 18, 2011 5:37 PM >> >> ... >> >> Actually, we used to follow strictly the pSOS convention for this >> >> until >> >> 2.4.x, at which point we moved to name strings precisely because >> >> non-null terminated char[4] arrays would break the registry, the way >> >> you >> >> described. This is one of the rare situations where mimicking a >> >> useless >> >> limitation of the original API may be challenged by usability concerns >> >> in the new environment, and usability won in that case. The problem >> >> mostly comes from the fact that char[4] is automatically convertible >> >> to >> >> const char *, so we have no warning/error leading us to check the >> >> potentially problematic call sites. >> >> >> >> The concern about moving back to char[4] arrays - null-terminated if >> >> shorter - is for people who currently assign strings longer than 4 >> >> characters to name their objects. What could be done, is providing a >> >> build switch to select the accepted input, like >> >> --disable-psos-long-names to turn on char[4] interpretation. >> > >> > While I would prefer the switch --enable-psos-long-names, this sounds >> > okay to me, the more so as it is more than people who use the pSOS >> > skin without obeying pSOS rules deserve. >> > -- >> >>> [..] >> > >> >> Another option would be to introduce a run-time switch. >> The default behavior could be that long names are supported and if the >> "enable_short_names()" function is called, all names will the cut at 4 >> characters. >> The advantage of this dynamic switch is that different applications >> using the same library can use the mode they prefer. >> >>> >> >>> I would also be in favor of a runtime setting, rather than a >> >>> compile-time one. One cannot assume that all PSOS applications on a >> >>> system follow the same rules, or that there cannot be a mix of >> >>> 'legacy' PSOS applications and 'xenomai-style' ones. According to me, >> >>> a library should support all these uses. >> >> >> >> Hi, >> >> >> >> here is a patch which truncates identifiers to four characters and >> >> terminates them by a '\0' character, in order to avoid the issues at >> >> the origin of this thread. The patch also adds a variable >> >> "psos_long_names", 0 by default, which may be set to a non-zero value >> >> to enable the old behaviour (assume the identifier strings may be >> >> longer than 4 characters and are already null terminated). >> >> >> >> Could someone with the issue test it? >> > >> > It seems this slipped through the cracks, my apologies. We already >> > implemented this ourselves similarly, we didn't actually test your >> > patch so far. >> > >> > Now that we're trying out xenomai-forge, I ported this patch and tested it. >> > There are a few problems with it (also relevant to cobalt xenomai) >> > >> > * a bug in __psos_maybe_short_name so that only 3 characters are >> > retained (see below) >> > * the call to __psos_maybe_short_name also needs to be done in the >> > _ident functions >> > * although we don't use it, I think the pt_create and pt_ident >> > functions also need to be adapted >> > >> > Moreover, I wonder why you have made psos_long_names an exported >> > global variable. Sharing variables between two different pieces of >> > software is not very clean. I think it would be nicer with a get/set >> > function. >> >> Thanks for the review. The exported global variable is the simplest >> possible and sufficient implementation of this feature. You risk having >> a hard time convincing us otherwise. > > Ack. > >> >> -- >> Gilles. >> >> ___ >> Xenomai-help mailing list >> Xenomai-help@gna.org >> https://mail.gna.org/listinfo/xenomai-help > > -- > Philippe. > > > > ___ > Xenomai-help mailing list > Xenomai-help@gna.org > https://mail.gna.org/listinfo/xenomai-help > Another simple solution would be to have a function available to set this mode. In that way the internal implementation can be changed without impact on the applications. I think it is always good to use a setter since this is more future safe. We also implemented the function tm_getm to get a
Re: [Xenomai-help] Command code working with comedi not working with analogy
El 11 de octubre de 2011 19:12, Alexis Berlemont escribió: [...] > I took some time to compare both versions of code (comedi and > analogy). I did not find anything interesting in mite.c. I was about > to ask you to increase verbosity (debug + a specific patch) when I got > a glimpse on the allocation of the asynchronous buffer on the comedi > side. > > The methods are not the same at that level: > - comedi: n * dma_alloc_coherent + a vmap at the end > - analogy: a big vmalloc + n * page_to_phys(vmalloc_to_page(vaddr) > Hmmm, quoting http://www.mjmwired.net/kernel/Documentation/DMA-mapping.txt: If you acquired your memory via the page allocator (i.e. __get_free_page*()) or the generic memory allocators (i.e. kmalloc() or kmem_cache_alloc()) then you may DMA to/from that memory using the addresses returned from those routines. This means specifically that you may _not_ use the memory/addresses returned from vmalloc() for DMA. It is possible to DMA to the _underlying_ memory mapped into a vmalloc() area, but this requires walking page tables to get the physical addresses, and then translating each of those pages back to a kernel address using something like __va(). [ EDIT: Update this when we integrate Gerd Knorr's generic code which does this. ] So, I guess analogy indeed took the walking approach mentioned there? If I understand it right, the following loop in "a4l_buf_alloc()": for (vaddr = vabase; vaddr < vabase + buf_desc->size; vaddr += PAGE_SIZE) buf_desc->pg_list[(vaddr - vabase) >> PAGE_SHIFT] = (unsigned long) page_to_phys(vmalloc_to_page(vaddr)); does exactly this, by holding a list of the physical addresses of all the logical pages of the buffer, even if they may be non-contiguous. Then, the MITE is able to scatter data across the ring descriptors calculated in a4l_mite_buf_change()? What is the benefit of using vmalloc? Copying from/to user space is easier so? According to my previous test, the addresses calculated are all indeed larger than 2^32. This makes sense as well, since this machine appears to have 6GB of memory: [0.00] Memory: 5992084k/7208960k available (5325k kernel code, 919428k absent, 297448k reserved, 3285k data, 920k init) The comedi drivers and kernel were not installed by myself, so reinstalling them is somewhat more involved. If you still feel it would be useful to check them out I will reinstall them, but this looks to me like the possible source of the problem. Best regards, Fernando ___ Xenomai-help mailing list Xenomai-help@gna.org https://mail.gna.org/listinfo/xenomai-help
Re: [Xenomai-help] Command code working with comedi not working with analogy [partially solved]
El 12 de octubre de 2011 16:13, Fernando Herrero Carrón escribió: > El 11 de octubre de 2011 19:12, Alexis Berlemont < > alexis.berlem...@gmail.com> escribió: > [...] > > >> I took some time to compare both versions of code (comedi and >> analogy). I did not find anything interesting in mite.c. I was about >> to ask you to increase verbosity (debug + a specific patch) when I got >> a glimpse on the allocation of the asynchronous buffer on the comedi >> side. >> >> The methods are not the same at that level: >> - comedi: n * dma_alloc_coherent + a vmap at the end >> - analogy: a big vmalloc + n * page_to_phys(vmalloc_to_page(vaddr) >> > > Hmmm, quoting http://www.mjmwired.net/kernel/Documentation/DMA-mapping.txt > : > > > If you acquired your memory via the page allocator > (i.e. __get_free_page*()) or the generic memory allocators > (i.e. kmalloc() or kmem_cache_alloc()) then you may DMA to/from > that memory using the addresses returned from those routines. > > This means specifically that you may _not_ use the memory/addresses > > > returned from vmalloc() for DMA. It is possible to DMA to the > _underlying_ memory mapped into a vmalloc() area, but this requires > walking page tables to get the physical addresses, and then > > > translating each of those pages back to a kernel address using > something like __va(). [ EDIT: Update this when we integrate > Gerd Knorr's generic code which does this. ] > > > So, I guess analogy indeed took the walking approach mentioned there? If I > understand it right, the following loop in "a4l_buf_alloc()": > > for (vaddr = vabase; vaddr < vabase + buf_desc->size; > vaddr += PAGE_SIZE) > buf_desc->pg_list[(vaddr - vabase) >> PAGE_SHIFT] = > (unsigned long) page_to_phys(vmalloc_to_page(vaddr)); > > does exactly this, by holding a list of the physical addresses of all the > logical pages of the buffer, even if they may be non-contiguous. Then, the > MITE is able to scatter data across the ring descriptors calculated in > a4l_mite_buf_change()? What is the benefit of using vmalloc? Copying from/to > user space is easier so? > > According to my previous test, the addresses calculated are all indeed > larger than 2^32. This makes sense as well, since this machine appears to > have 6GB of memory: > > [0.00] Memory: 5992084k/7208960k available (5325k kernel code, > 919428k absent, 297448k reserved, 3285k data, 920k init) > > The comedi drivers and kernel were not installed by myself, so reinstalling > them is somewhat more involved. If you still feel it would be useful to > check them out I will reinstall them, but this looks to me like the possible > source of the problem. > > I got it working!!! Simple test: remove two of the three RAM modules. Now the machine is working with 2GB of memory: [0.00] Memory: 1988808k/2095680k available (5325k kernel code, 452k absent, 106420k reserved, 3285k data, 920k init) Now "cmd_read" is properly acquiring the input signal. Output of dmesg now: [ 109.389613] Analogy: sizeof(dma_addr_t) = 8 [ 109.389614] Analogy: ring->descriptors_dma_addr = 7a279000 [ 109.389615] Analogy: cpu_to_le32(ring->descriptors_dma_addr) = 7a279000 [ 109.389617] Analogy: buf->pg_list[0] = 79322000 [ 109.389618] Analogy: buf->pg_list[1] = 799bf000 [ 109.389619] Analogy: buf->pg_list[2] = 79b67000 [ 109.389620] Analogy: buf->pg_list[3] = 79303000 [ 109.389621] Analogy: buf->pg_list[4] = 79015000 [ 109.389622] Analogy: buf->pg_list[5] = 7997f000 [ 109.389623] Analogy: buf->pg_list[6] = 792c1000 [ 109.389625] Analogy: buf->pg_list[7] = 792a7000 [ 109.389626] Analogy: buf->pg_list[8] = 7a087000 [ 109.389627] Analogy: buf->pg_list[9] = 792c [ 109.389628] Analogy: buf->pg_list[10] = 79b36000 [ 109.389629] Analogy: buf->pg_list[11] = 792b6000 [ 109.389630] Analogy: buf->pg_list[12] = 792d [ 109.389631] Analogy: buf->pg_list[13] = 7999d000 [ 109.389632] Analogy: buf->pg_list[14] = 7a1f7000 [ 109.389634] Analogy: buf->pg_list[15] = 791e with all pg_list[] entries below 2^32!! Thus far this does it for us, since we can live with a 4GB machine. I think the "vmalloc()" approach in analogy should be reworked, but my knowledge of linux's internals on memory handling is very limited. Please let me know if I can contribute testing any patches. Thank you both for your interest and willingness to help!! Sincerely, Fernando ___ Xenomai-help mailing list Xenomai-help@gna.org https://mail.gna.org/listinfo/xenomai-help
Re: [Xenomai-help] If you have any problems with health, this site will help you!
...I promise you will be on the top of pleasure! http://www.mbenislam.com/com.friend.php?efriend=60g4 ___ Xenomai-help mailing list Xenomai-help@gna.org https://mail.gna.org/listinfo/xenomai-help
Re: [Xenomai-help] PSOS skin: mismatch in function signatures cause buffer overflow
On Wed, 2011-10-12 at 19:24 +0200, Ronny Meeus wrote: > On Wed, Oct 12, 2011 at 5:56 PM, Philippe Gerum wrote: > > On Wed, 2011-10-12 at 16:22 +0200, Gilles Chanteperdrix wrote: > >> On 10/12/2011 04:03 PM, Thomas De Schampheleire wrote: > >> > Hi, > >> > > >> > On Mon, Sep 26, 2011 at 11:46 PM, Gilles Chanteperdrix > >> > wrote: > >> >> On 09/19/2011 10:45 AM, Thomas De Schampheleire wrote: > >> >>> Hi, > >> >>> > >> >>> On Mon, Sep 19, 2011 at 9:42 AM, Ronny Meeus > >> >>> wrote: > >> On Mon, Sep 19, 2011 at 9:25 AM, > >> wrote: > >> >> From: xenomai-help-boun...@gna.org > >> >> [mailto:xenomai-help-boun...@gna.org] > >> >> On Behalf Of Philippe Gerum > >> >> Sent: Sunday, September 18, 2011 5:37 PM > >> >> ... > >> >> Actually, we used to follow strictly the pSOS convention for this > >> >> until > >> >> 2.4.x, at which point we moved to name strings precisely because > >> >> non-null terminated char[4] arrays would break the registry, the > >> >> way you > >> >> described. This is one of the rare situations where mimicking a > >> >> useless > >> >> limitation of the original API may be challenged by usability > >> >> concerns > >> >> in the new environment, and usability won in that case. The problem > >> >> mostly comes from the fact that char[4] is automatically > >> >> convertible to > >> >> const char *, so we have no warning/error leading us to check the > >> >> potentially problematic call sites. > >> >> > >> >> The concern about moving back to char[4] arrays - null-terminated if > >> >> shorter - is for people who currently assign strings longer than 4 > >> >> characters to name their objects. What could be done, is providing a > >> >> build switch to select the accepted input, like > >> >> --disable-psos-long-names to turn on char[4] interpretation. > >> > > >> > While I would prefer the switch --enable-psos-long-names, this > >> > sounds okay to me, the more so as it is more than people who use the > >> > pSOS skin without obeying pSOS rules deserve. > >> > -- > >> >>> [..] > >> > > >> > >> Another option would be to introduce a run-time switch. > >> The default behavior could be that long names are supported and if the > >> "enable_short_names()" function is called, all names will the cut at 4 > >> characters. > >> The advantage of this dynamic switch is that different applications > >> using the same library can use the mode they prefer. > >> >>> > >> >>> I would also be in favor of a runtime setting, rather than a > >> >>> compile-time one. One cannot assume that all PSOS applications on a > >> >>> system follow the same rules, or that there cannot be a mix of > >> >>> 'legacy' PSOS applications and 'xenomai-style' ones. According to me, > >> >>> a library should support all these uses. > >> >> > >> >> Hi, > >> >> > >> >> here is a patch which truncates identifiers to four characters and > >> >> terminates them by a '\0' character, in order to avoid the issues at > >> >> the origin of this thread. The patch also adds a variable > >> >> "psos_long_names", 0 by default, which may be set to a non-zero value > >> >> to enable the old behaviour (assume the identifier strings may be > >> >> longer than 4 characters and are already null terminated). > >> >> > >> >> Could someone with the issue test it? > >> > > >> > It seems this slipped through the cracks, my apologies. We already > >> > implemented this ourselves similarly, we didn't actually test your > >> > patch so far. > >> > > >> > Now that we're trying out xenomai-forge, I ported this patch and tested > >> > it. > >> > There are a few problems with it (also relevant to cobalt xenomai) > >> > > >> > * a bug in __psos_maybe_short_name so that only 3 characters are > >> > retained (see below) > >> > * the call to __psos_maybe_short_name also needs to be done in the > >> > _ident functions > >> > * although we don't use it, I think the pt_create and pt_ident > >> > functions also need to be adapted > >> > > >> > Moreover, I wonder why you have made psos_long_names an exported > >> > global variable. Sharing variables between two different pieces of > >> > software is not very clean. I think it would be nicer with a get/set > >> > function. > >> > >> Thanks for the review. The exported global variable is the simplest > >> possible and sufficient implementation of this feature. You risk having > >> a hard time convincing us otherwise. > > > > Ack. > > > >> > >> -- > >> Gilles. > >> > >> ___ > >> Xenomai-help mailing list > >> Xenomai-help@gna.org > >> https://mail.gna.org/listinfo/xenomai-help > > > > -- > > Philippe. > > > > > > > > ___ > > Xenomai-help mailing list > > Xenomai-help@gna.org > > https://mail.gna.org/listinfo/xenomai-hel
Re: [Xenomai-help] Command code working with comedi not working with analogy [partially solved]
Hi 2011/10/12 Fernando Herrero Carrón : > El 12 de octubre de 2011 16:13, Fernando Herrero Carrón > escribió: >> >> El 11 de octubre de 2011 19:12, Alexis Berlemont >> escribió: >> [...] >> >>> >>> I took some time to compare both versions of code (comedi and >>> analogy). I did not find anything interesting in mite.c. I was about >>> to ask you to increase verbosity (debug + a specific patch) when I got >>> a glimpse on the allocation of the asynchronous buffer on the comedi >>> side. >>> >>> The methods are not the same at that level: >>> - comedi: n * dma_alloc_coherent + a vmap at the end >>> - analogy: a big vmalloc + n * page_to_phys(vmalloc_to_page(vaddr) >> >> Hmmm, quoting >> http://www.mjmwired.net/kernel/Documentation/DMA-mapping.txt: >> >> If you acquired your memory via the page allocator >> (i.e. __get_free_page*()) or the generic memory allocators >> >> (i.e. kmalloc() or kmem_cache_alloc()) then you may DMA to/from >> that memory using the addresses returned from those routines. >> >> This means specifically that you may _not_ use the memory/addresses >> >> >> >> >> returned from vmalloc() for DMA. It is possible to DMA to the >> _underlying_ memory mapped into a vmalloc() area, but this requires >> walking page tables to get the physical addresses, and then >> >> >> >> >> translating each of those pages back to a kernel address using >> something like __va(). [ EDIT: Update this when we integrate >> Gerd Knorr's generic code which does this. ] >> >> So, I guess analogy indeed took the walking approach mentioned there? If I >> understand it right, the following loop in "a4l_buf_alloc()": >> >> for (vaddr = vabase; vaddr < vabase + buf_desc->size; >> vaddr += PAGE_SIZE) >> buf_desc->pg_list[(vaddr - vabase) >> PAGE_SHIFT] = >> (unsigned long) page_to_phys(vmalloc_to_page(vaddr)); >> >> does exactly this, by holding a list of the physical addresses of all the >> logical pages of the buffer, even if they may be non-contiguous. Then, the >> MITE is able to scatter data across the ring descriptors calculated in >> a4l_mite_buf_change()? What is the benefit of using vmalloc? Copying from/to >> user space is easier so? >> >> According to my previous test, the addresses calculated are all indeed >> larger than 2^32. This makes sense as well, since this machine appears to >> have 6GB of memory: >> >> [ 0.00] Memory: 5992084k/7208960k available (5325k kernel code, >> 919428k absent, 297448k reserved, 3285k data, 920k init) >> >> The comedi drivers and kernel were not installed by myself, so >> reinstalling them is somewhat more involved. If you still feel it would be >> useful to check them out I will reinstall them, but this looks to me like >> the possible source of the problem. >> > > I got it working!!! Simple test: remove two of the three RAM modules. Now > the machine is working with 2GB of memory: OK. > [ 0.00] Memory: 1988808k/2095680k available (5325k kernel code, 452k > absent, 106420k reserved, 3285k data, 920k init) > > Now "cmd_read" is properly acquiring the input signal. Output of dmesg now: > > [ 109.389613] Analogy: sizeof(dma_addr_t) = 8 > [ 109.389614] Analogy: ring->descriptors_dma_addr = 7a279000 > [ 109.389615] Analogy: cpu_to_le32(ring->descriptors_dma_addr) = 7a279000 > [ 109.389617] Analogy: buf->pg_list[0] = 79322000 > [ 109.389618] Analogy: buf->pg_list[1] = 799bf000 > [ 109.389619] Analogy: buf->pg_list[2] = 79b67000 > [ 109.389620] Analogy: buf->pg_list[3] = 79303000 > [ 109.389621] Analogy: buf->pg_list[4] = 79015000 > [ 109.389622] Analogy: buf->pg_list[5] = 7997f000 > [ 109.389623] Analogy: buf->pg_list[6] = 792c1000 > [ 109.389625] Analogy: buf->pg_list[7] = 792a7000 > [ 109.389626] Analogy: buf->pg_list[8] = 7a087000 > [ 109.389627] Analogy: buf->pg_list[9] = 792c > [ 109.389628] Analogy: buf->pg_list[10] = 79b36000 > [ 109.389629] Analogy: buf->pg_list[11] = 792b6000 > [ 109.389630] Analogy: buf->pg_list[12] = 792d > [ 109.389631] Analogy: buf->pg_list[13] = 7999d000 > [ 109.389632] Analogy: buf->pg_list[14] = 7a1f7000 > [ 109.389634] Analogy: buf->pg_list[15] = 791e > > with all pg_list[] entries below 2^32!! > > Thus far this does it for us, since we can live with a 4GB machine. I think > the "vmalloc()" approach in analogy should be reworked, but my knowledge of > linux's internals on memory handling is very limited. Please let me know if > I can contribute testing any patches. I am modfiying the asynchronous buffer allocation stuff right now. I will provide a patch as soon as it is working on my side. > Thank you both for your interest and willingness to help!! > > Sincerely, > Fernando > > ___ > Xenomai-help mailing list > Xenomai-help@gna.org > https://mail.gna.org/listinfo/xenomai-help > > Alexis. ___ Xenomai-help mailing list Xenomai-help@gna.org https://mail.gna.org/listinfo/xenomai-he
