net/gnrc/pktbuf: Add gnrc_pktbuf_merge

[tobhe]

This adds a gnrc_pktbuf_merge(gnrc_pktsnip_t *pkt) function.
It's main use is pulling data together in the memory to be able to use hash or crypto functions on the packet which take consecutive byte arrays as an input. This should make it easier to implement protocols like IKE, ESP, DTLS and others using MAC.

[miri64]

A few questions:

1. Don't most crypto functions have some kind of "update" functionality, that allows for cumulative encryption? (In that case you don't need this functionality)
2. Why are the gnrc_pktsnip_t thrown away?

[tobhe]

@miri64

Don't most crypto functions have some kind of "update" functionality, that allows for cumulative encryption? (In that case you don't need this functionality)

Actually i don't know of any, at least RIOT's crypto module doesn't allow this. Even more i guess it would still be useful to be able to use external crypto lib's functions.

Why are the gnrc_pktsnip_t thrown away?

I actually tried this first by just changing their pointers to the right position in the new data segment, it all worked well but test cases showed some problems after releasing the packets. As far as i got it the reason was _pktbuf_free assuming every data block is aligned, which is obviously not possible without having a bunch of 0s in your data block, so i decided the easiest solution would be to treat it as a single pktsnip.

[smlng]

I don't see the benefits of this - actually I think it might also be problematic to merge and remove snips afterwards, @miri64 seems to be skeptical, too .

If such functionality is really needed by some crypto libraries, I'd rather have an intermediate function to merge the buffers into a single buffer (without destroying the snips), something like

int gnrc_pktbuf_merge (gnrc_pktsnip_t *pkt, uint8_t *buf, size_t buflen)
{
    size_t count = 0;
    while (pkt && ((count + pkt->size) < buflen)) {
        if (pkt->data) {
            memcpy(buf+count, pkt->data, pkt->size)
            count += pkt->size
        }
        pkt = pkt->next;
    }
}

[tobhe]

Thanks for sharing your thoughts @smlng

I think it might also be problematic to merge and remove snips afterwards

I do understand your worries, but i don't actually think there should be a problem. The tests cases show no technical problems, so i guess the thing you're worried about is the loss of Information(please correct me if I got that wrong), which might seem strange or unnecessary on the first sight, but if you think about it at the point where you are hashing or encrypting that packet you cannot change anything in that chunk of data anyway without rendering your hash or cipher obsolete.

If such functionality is really needed by some crypto libraries

I think you got that wrong from my previous comment, what i was saying was it would still be useful if it was just some, but actually right now we're talking about every single crypto library that i know of that it's available on riot[1][2][3][4], the only exception which uses update is RIOT's hashes module [5]

I'd rather have an intermediate function to merge the buffers into a single buffer (without destroying the snips), something like

int gnrc_pktbuf_merge (gnrc_pktsnip_t *pkt, uint8_t *buf, size_t buflen)
{
    size_t count = 0;
  while (pkt && ((count + pkt->size) < buflen)) {
       if (pkt->data) {
           memcpy(buf+count, pkt->data, pkt->size)
           count += pkt->size
       }
       pkt = pkt->next;
   }
}

The main problem i see here is the need to manage another buffer for a packet whose size might not be static so you would have to just allocate the maximum possible size, while the pktbuf is there too, already allocated but not used, made especially for this. So it's just quite memory efficient to not use the packetbuffer in my opinion.

References:
[1] RIOT Crypto
[2] tweetnacl
[3] tinydtls
[4] Wolfssl API
[5] RIOT Hashes MD5

[dobby880]

I'm supervising @tobhe on this project.
I can see your concerns about loosing information. However, going through the source code, there are places were security is left as a TODO (e.g. 802.15.4), where it could be helpful to have this functionality.

I believe the pktbuf should allow crypto-functions on the packet, regardless how. @tobhe showed one way to do that, which doesn't impact the current pktbuf a lot.
The alternative (and maybe better) solution would be to split the pktbuf into two, one for the linked list and one for the data (without 32 bit alignment). As the allocation for data is seperate to the allocation for the linked-list-information, that shouldn't be a proplem either. But maybe that has impact on other parts of RIOT, we can't overlook for now, as we're not as deep into RIOT as you.

Just for interest: Is there a certain reason for the 32bit alignment in the pktbuf? Without this, keeping the snips would be easy, as @miri64 mentioned.

[miri64]

Just for interest: Is there a certain reason for the 32bit alignment in the pktbuf? Without this, keeping the snips would be easy, as @miri64 mentioned.

Yes, on some of our supported platforms (e.g. all Cortex-M0+ platforms) accessing a[edit]an unaligned[/edit] pointer (i.e. the beginning of a buffer if you cast it to a header struct) may solicit the CPU to crash.

[tobhe]

Accidental close...

[tobhe]

@miri64 how do you think about having a gnrc_pktbuf_duplicate(gnrc_pktsnip_t *pkt) instead. It could do pretty much the same thing as gnrc_pktbuf_duplicate_upto(gnrc_pktsnip_t *pkt, gnrc_nettype_t type), for the whole packet without you being required to state a type. This would allow you to keep the old one or just free it if you don't think you'll need it anymore.

[kaspar030]

If such functionality is really needed by some crypto libraries, I'd rather have an intermediate function to merge the buffers into a single buffer (without destroying the snips), something like

+1

Whatever comes next, this would probably be an intermediate step anyways. If the pktbuf cannot hold the whole flattened packet as one piece, merging the snips fails anyways.

[smlng]

@kaspar030: good point, and maybe the real issue here. Could be worth investigating the fragmentation of the packet buffer over time (in a realistic scenario) and how likely (or unlikely) it is to find a large enough section to fit a merged (full) packet.

[miri64]

@miri64 how do you think about having a gnrc_pktbuf_duplicate(gnrc_pktsnip_t *pkt) instead. It could do pretty much the same thing as gnrc_pktbuf_duplicate_upto(gnrc_pktsnip_t *pkt, gnrc_nettype_t type), for the whole packet without you being required to state a type. This would allow you to keep the old one or just free it if you don't think you'll need it anymore.

Well gnrc_pktbuf_start_write() already provides a functionality that duplicates a packet implicitly. But maybe a more explicit relative to gnrc_pktbuf_duplicate_upto() might indeed be a good idea.

[dobby880]

If such functionality is really needed by some crypto libraries, I'd rather have an intermediate function to merge the buffers into a single buffer (without destroying the snips), something like

+1
Whatever comes next, this would probably be an intermediate step anyways. If the pktbuf cannot hold the whole flattened packet as one piece, merging the snips fails anyways.

This is also true for gnrc_pktbuf_duplicate_upto()
If that is really the issue here, we need some kind of a sort function for the buffer, which is more complex.

Well gnrc_pktbuf_start_write() already provides a functionality that duplicates a packet implicitly. But maybe a more explicit relative to gnrc_pktbuf_duplicate_upto() might indeed be a good idea.

+1
but this requires you (or the user) making sure that before dispatchthe not dispatched pktsnip is freed.

[miri64]

As discussed F2F, I don't see, why this needs to be implemented in gnrc_pktbuf_static. A helper function in the common gnrc_pktbuf would be better suited and allows for easier integration of later alternatives to gnrc_pktbuf_static.

[miri64]

Ping @tobhe?

[tobhe]

@miri64: I'm still there, I just didn't find time for this yet.

[smlng]

from the discussion I take it this needs some work, hence won't make it for release 2018.01

[miri64]

Ping @tobhe. Or did you abandon this in favor for #7651?

[tobhe]

@miri64 not abandoned, rather neglected due to #7651 ;) but yes, this is still useful and i will have to take care of this sometime soon

[miri64]

@tobhe BTW I'm planning to provide an alternative gnrc_pktbuf wrapper sometime after #7651 is merged for it (similar to gnrc_pktbuf_malloc, so there is a second incentive to make this function independent of gnrc_pktbuf_static ;-).

[tobhe]

Good to hear, sounds awesome! Yes, I think in any case it makes sense to put it with the other helpers as there are no direct dependencies on the static part, I even think i have some local branch somewhere where I already did this. The thing that stopped me was the tests if i remember correctly 😄

[tcschmidt]

@tobhe any plans to return to this?

[tobhe]

@tcschmidt i am still planning to finish this, but i don't know when i will find the time. We can close it down for now and i will just open a new request once i got something.

[tcschmidt]

@tobhe O.K., sounds like a plan. It's propably best to contact @miri64 early once you plan to advance this to a production state.

[tcschmidt]

Closed until author will take up.