buglwIP - A Lightweight TCP/IP stack - Bugs: bug #48300, Private mempools allocate foreign...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #48300: Private mempools allocate foreign memory

Submitter:  chrysn <chrysn>
Submitted:  Thu 23 Jun 2016 03:00:05 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  2.0.0 Beta2
lwIP version:  git head

Jump to the original submission

Thu 07 Jul 2016 11:40:56 AM UTC, comment #11: 

Fixed in another way as your proposal, but should work for you, too

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 06 Jul 2016 10:57:53 AM UTC, comment #10: 

In assembling the patch, I found that there has been already a start of the mechanisms required in place -- the LWIP_MEM_ALIGN_BUFFER macro already did the right thing by adding (alignment-1) bytes to a size, it was just never used, so a first patch makes candidates use it.

The second patch introduces a macro LWIP_ALIGN_SUFFIX to cc.h. If there is a way to make the compiler align memory at build time, LWIP_MEM_ALIGN_BUFFER will not overallocate memory. The macro's documentation has been updated, it is now required for all users of it to use LWIP_ALIGN_SUFFIX as well. This introduces a dependency from lwip/mem.h to lwip/arch.h -- I hope that's not a problem, I was unsure of which path cc.h is usually included via.

We could go a tad further and, at all locations where alignment through this mechanism is enforced make the alignment calls assertations (as trouble is to come when LWIP_ALIGN_SUFFIX is advertised but doesn't work) and in optimized builds no-ops; I do have a patch for that too but it's preview quality only and would need further discussion. That patch has been useful for testing, though.

Speaking of testing: I'd like to add one or two test cases to the whole allocation thing, but found no documentation on the tests/unit directory. How are additional tests created, and how is the test suite run?

One thing that did occur to me when I reviewed all alignment macro uses is that I don't quite see why so much effort is put into aligning the result of pbuf creating functions -- after the next pbuf_header(), things might be out of alignment, and whoever would rely on pbufs being aligned could only ever work with pbufs they themselves have created, for every other pbuf might have different alignment. IMO it would be more advantageous to have the IP header aligned, but not whichever-level-the-pbuf-was-created-on.

(file #37729, file #37730, file #37731)

chrysn <chrysn>
Sun 03 Jul 2016 07:58:07 PM UTC, comment #9: 

To comment #7: Does the waste of memory really matter? We pad each pool element to MEM_ALIGNMENT - which means we waste NumberOfElements*(MEM_ALIGNMENT/2) in the average case anyway.

I'm only saying this because it is a counter-argument to introducing yet another define...

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 01 Jul 2016 05:34:48 AM UTC, comment #8: 

I'm just going through the uses of MEM_ALIGN*, and I am preparing a proposed patch (that would save memory if cc.h provides a way to align allocations, and otherwise keep allocating too much) over the weekend.

chrysn <chrysn>
Thu 30 Jun 2016 08:23:33 PM UTC, comment #7: 

I'd like to fix this properly before releasing the next beta: we don't support allocating all pools in one block any more, so we always allocate "MEM_ALIGNMENT-1" bytes too much for each pool...

Simon Goldschmidt <goldsimon>
Group administrator
Fri 24 Jun 2016 01:14:08 PM UTC, comment #6: 

Adding these bytes is a waste of memory. A better fix would be to enforce alignment on these buffers. We would need PORTABLE alignment macros like for packing...

Does anyone have a PORTABLE solution?

- gcc
- IAR
- MSVC
- ...

Maybe using a naturally aligned datatype would be a solution? e.g. declaring the pool as u32_t. For your actual problem, you need to use uint64_t (AFAIK at least ARM aligns it to 8 byte for FPU reasons).

#define LWIP_MEMPOOL_DECLARE(name,num,size,desc) LWIP_MEMPOOL_TYPE memp_memory_ ## name ## _base \
[(((num) * (MEMP_SIZE + MEMP_ALIGN_SIZE(size)))) / sizeof(LWIP_MEMPOOL_TYPE)]; \

Could you please try it?

Others, any comments to this (a little bit hackerish) solution?


Dirk Ziegelmeier <dziegel>
Group administrator
Fri 24 Jun 2016 11:06:07 AM UTC, comment #5: 

The solution in 2c1bd36 works; thank you for the fast fix!

chrysn <chrysn>
Fri 24 Jun 2016 08:48:16 AM UTC, comment #4: 

#define LWIP_MEMPOOL_DECLARE(name,num,size,desc) u8_t memp_memory_ ## name ## _base \
    [((num) * (MEMP_SIZE + MEMP_ALIGN_SIZE(size))) + (MEM_ALIGNMENT-1)]; \
[...]

should work, but is a waste of memory. I'm currently discussing with Simon whether we have a good idea.

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 24 Jun 2016 07:29:24 AM UTC, comment #3: 

The patch did not have the desired effect; the symptom persists, and applying it didn't even change the symbol table (`nm -S` output); given I don't have memp debugging enabled (which might probably catch the issue), MEMP_SIZE is 0, so the `(MEMP_SIZE + MEMP_ALIGN(size))` term is already rounded, so thus is `(num) * ...`.

Reviewing other uses of LWIP_MEM_ALIGN_SIZE: ip_frag.c's u8_t buf enlarges itself by MEM_ALIGN_SIZE - 1 (which is roughly equivalent to one of my original proposals; I think the `-1` is a good catch because it's sufficient, but having the extra space inside the LWIP_MEM_ALIGN_SIZE macro caues unnecessary increases -- details, though, I could be wrong about them and would need to do the math thoroughly). The idiom seems suitable, all in all, and was introduced back then for the very same reason (see 96b788bea7fb8).

I suggest we use that as a quick fix, and introduce a more precise mechanism for memp as well as ip_frag's buf when a portable formulation of `__attribute__((aligned(n)))` was found. (That would depend on users of other compilers than gcc to chime in).

chrysn <chrysn>
Thu 23 Jun 2016 06:22:19 PM UTC, comment #2: 

Thank you for reporting. Please check whether the fix I just committed works for you.

Dirk Ziegelmeier <dziegel>
Group administrator
Thu 23 Jun 2016 03:31:23 PM UTC, comment #1: 

The issue can be worked around by adding `__attribute__((aligned(MEM_ALIGNMENT)))` after the u8_t of LWIP_MEMPOOL_DECLARE -- but that's a GCC'ism, and as I only see those in cc.h for `PACK_STRUCT_STRUCT`, I assume that the general intention is to keep such compiler-isms there; the hard thing here (as compared to `PACK_STRUCT_STRUCT`, which I assume is only used for saving memory) is that such a feature can't be generally assumed to be present on all compilers.

I see three possible courses of action:

  • Add an ALIGN macro which would need to be defined in all users' `cc.h` to keep this subtle bug from resurfacing. (No falling back to ignoring the alignment unless MEM_ALIGNMENT = 1).
  • Unconditionally add MEM_ALIGNMENT bytes at the end of pools to compensate for any possible offset.
  • Do the latter conditionally if the former is not implemented.


Which would you prefer?

chrysn <chrysn>
Thu 23 Jun 2016 03:00:05 PM UTC, original submission:  

The memory access to pools declared using LWIP_MEMPOOL_DECLARE can be flawed when MEM_ALIGNMENT is used. This was observed on an ARM Cortex device on which MEM_ALIGNMENT was set to 8 for reasons outside the architecture. Two pools are allocated in parallel, and memset'ing what was obtained from one of the pools wiped the tab of the other.

I suppose this is because memp_memory_*_base itself is an unaligned u8_t array, but in memp_init_pool, the address of desc->base gets aligned and thus potentially increased; fully accessing the last element would then go to an area not reserved to the respective _base.

I'll see whether I can submit a viable patch.

chrysn <chrysn>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #37729:  0001-Use-LWIP_MEM_ALIGN_BUFFER-macro.patch added by chrysn (2KiB - application/octet-stream)
file #37730:  0002-Introduce-LWIP_ALIGN_SUFFIX-macro.patch added by chrysn (4KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Updated the item)
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by chrysn (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-07 dziegel StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-07-06 chrysn Attached File- Added 0001-Use-LWIP_MEM_ALIGN_BUFFER-macro.patch, #37729
        Attached File- Added 0002-Introduce-LWIP_ALIGN_SUFFIX-macro.patch, #37730
        Attached File- Added 0003-demo-only-Be-strict-about-when-memory-should-already.patch, #37731
    2016-06-30 goldsimon StatusReady For Test In Progress
    2016-06-30 goldsimon Planned ReleaseNone 2.0.0 Beta2
    2016-06-27 goldsimon Planned Release 2.0.0 Beta2
    2016-06-24 dziegel StatusConfirmed Ready For Test
    2016-06-24 dziegel StatusFixed Confirmed
        Open/ClosedClosed Open
    2016-06-23 dziegel StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code