linux-toolchains.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Cufi, Carles" <Carles.Cufi@nordicsemi.no>
To: David Laight <David.Laight@ACULAB.COM>,
	'Florian Weimer' <fweimer@redhat.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"jukka.rissanen@linux.intel.com" <jukka.rissanen@linux.intel.com>,
	"johan.hedberg@intel.com" <johan.hedberg@intel.com>,
	"Lubos, Robert" <Robert.Lubos@nordicsemi.no>,
	"Bursztyka, Tomasz" <tomasz.bursztyka@intel.com>,
	"linux-toolchains@vger.kernel.org"
	<linux-toolchains@vger.kernel.org>
Subject: RE: Non-packed structures in IP headers
Date: Mon, 4 Oct 2021 10:41:38 +0000	[thread overview]
Message-ID: <DB9PR05MB789852210DBB2F6264536A8EE7AE9@DB9PR05MB7898.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <a8082bcaeb534ee5b24ea6dae4428547@AcuMS.aculab.com>

> From: David Laight <David.Laight@ACULAB.COM>
> Sent: 02 October 2021 17:55
> From: Florian Weimer
> > Sent: 01 October 2021 21:10
> >
> > * Carles Cufi:
> >
> > > I was looking through the structures for IPv{4,6} packet headers and
> > > noticed that several of those that seem to be used to parse a packet
> > > directly from the wire are not declared as packed. This surprised me
> > > because, although I did find that provisions are made so that the
> > > alignment of the structure, it is still technically possible for the
> > > compiler to inject padding bytes inside those structures, since
> > > AFAIK the C standard makes no guarantees about padding unless it's
> > > instructed to pack the structure.
> >
> > The C standards do not make such guarantees, but the platform ABI
> > standards describe struct layout and ensure that there is no padding.
> > Linux relies on that not just for networking, but also for the
> > userspace ABI, support for separately compiled kernel modules, and in
> > other places.
> 
> In particular structures are used to map hardware register blocks.

Non-padded ones? Because this again might be an issue depending on the compiler/ABI as per my understanding.

> 
> > Sometimes there are alignment concerns in the way these structs are
> > used, but I believe the kernel generally controls placement of the
> > data that is being worked on, so that does not matter, either.
> >
> > Therefore, I do not believe this is an actual problem.
> 
> And adding __packed forces the compiler to do byte accesses (with shifts)
> on cpu that don't support misaligned memory accesses.

Right, I understand that using __packed involves a runtime penalty hit on memory accesses, but I wasn't proposing to pack those structs, just to check their size for padding at compile-time.

> So it really is wrong to specify __packed unless the structure can be
> unaligned in memory, or has a 'broken' definition that has fields that
> aren't 'naturally aligned'.

But who defines what "broken" or "natural alignment" is for each architecture? Furthermore, the C standard doesn't really mention any of these terms as far as I know, it just leaves complete freedom to the compiler to add the padding it might consider appropriate.

> In the latter case it is enough to mark the field that requires the
> padding before it removed as (IIRC) __aligned(1).
> The compiler will then remove the padding but still assume the field is
> partially aligned - so my do two 32bit access instead of 8 8bit ones).

Interesting, I have never seen this used before, but then again I come from an (deeply) embedded background that tends to pack all of its structs that represent bytes that go over the wire (or hardware registers for that matter).

What is also interesting in this case, is why the ethernet header is packed in Linux[2]. There don't seem to be any special alignment or size constraints when compared to the IP headers, since the single 16-bit integer is placed 12 bytes after the beginning of the struct. I assume the reason for packing this header is indeed alignment, and not padding. Unlike the IP headers, the kernel probably doesn't ensure that an ethernet header begins at an address compatible with the alignment requirements of that 16-bit integer, so the header needs packing not because there's a risk of padding being introduced by the compiler, but because the header might start at an odd memory address?

Thanks,

Carles

[2] https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/if_ether.h#L169

  reply	other threads:[~2021-10-04 10:41 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <AS8PR05MB78952FE7E8D82245D309DEBCE7AA9@AS8PR05MB7895.eurprd05.prod.outlook.com>
2021-10-01 20:10 ` Non-packed structures in IP headers Florian Weimer
2021-10-02 15:54   ` David Laight
2021-10-04 10:41     ` Cufi, Carles [this message]
2021-10-04 12:18       ` David Laight
2021-10-04 10:30   ` Cufi, Carles
2021-10-09  6:56     ` Florian Weimer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DB9PR05MB789852210DBB2F6264536A8EE7AE9@DB9PR05MB7898.eurprd05.prod.outlook.com \
    --to=carles.cufi@nordicsemi.no \
    --cc=David.Laight@ACULAB.COM \
    --cc=Robert.Lubos@nordicsemi.no \
    --cc=fweimer@redhat.com \
    --cc=johan.hedberg@intel.com \
    --cc=jukka.rissanen@linux.intel.com \
    --cc=linux-toolchains@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=tomasz.bursztyka@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).