linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Theodore Ts'o" <tytso@mit.edu>
To: harshad shirwadkar <harshadshirwadkar@gmail.com>
Cc: Andreas Dilger <adilger@dilger.ca>,
	Ext4 Developers List <linux-ext4@vger.kernel.org>,
	Harshad Shirwadkar <harshads@google.com>
Subject: Re: [PATCH] e2fsck: fix portability problems caused by unaligned accesses
Date: Tue, 4 May 2021 09:49:15 -0400	[thread overview]
Message-ID: <YJFQ20rLK16rise2@mit.edu> (raw)
In-Reply-To: <CAD+ocbx9STMGrE0xkHtR8J_c_TgMEz1A6MmNOQyrQtakoZjq3Q@mail.gmail.com>

On Tue, May 04, 2021 at 02:40:08AM -0700, harshad shirwadkar wrote:
> Hi Ted,
> 
> Thanks for the patch. While I now see that these accesses are safe,
> ubsan still complains about it the dereferences not being aligned.
> With your changes, the way we read journal_block_tag_t is now safe.
> But IIUC, ubsan still complains mainly because we still pass the
> pointer as "&tag->t_flags" and at which point ubsan thinks that we are
> accessing member t_flags in an aligned way. Is there a way to silence
> these errors?

Yeah, I had noticed that.  I was thinking perhaps of doing something
like casting the pointer to void * or char *, and then adding offsetof
to work around the UBSAN warning.  Or maybe asking the compiler folks
if they can make the UBSAN warning smarter, since what we're doing
should be perfectly safe. 

> 
> I was wondering if it makes sense to do something like this for known
> unaligned structures:
> 
> journal_block_tag_t local, *unaligned;
> ...
> memcpy(&local, unaligned, sizeof(&local));

I guess that would work too.  The extra memory copy is unfortunate,
although I suspect the performance hit isn't measurable, and journal
replay isn't really a hot path in either the kernel or e2fsprogs.
(Note that want to keep recovery.c in sync between the kernel and
e2fsprogs, so whatever we do needs to be something we're happy with in
both places.)

						- Ted

  reply	other threads:[~2021-05-04 13:49 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-04  3:10 [PATCH] e2fsck: fix portability problems caused by unaligned accesses Theodore Ts'o
2021-05-04  6:29 ` Andreas Dilger
2021-05-04  9:40   ` harshad shirwadkar
2021-05-04 13:49     ` Theodore Ts'o [this message]
2021-05-04 16:46       ` Eric Biggers
2021-05-04 17:55         ` harshad shirwadkar
2021-05-04 19:14           ` Eric Biggers
2021-05-04 19:53             ` Theodore Ts'o
2021-05-04 20:14               ` harshad shirwadkar
2021-05-04 20:45                 ` Eric Biggers
2021-05-04 21:10                   ` Theodore Ts'o
2021-05-04 21:30                     ` Eric Biggers
2021-05-07  6:45                       ` Eric Biggers
2021-05-07 15:56                         ` Theodore Ts'o
2021-05-07 16:22                           ` harshad shirwadkar
2021-05-04 20:35               ` Eric Biggers
2021-05-04 15:18   ` Theodore Ts'o
2021-05-06 18:30 ` [PATCH -v2] " Theodore Ts'o
2021-05-06 23:30   ` harshad shirwadkar
2021-05-07  1:50     ` Theodore Ts'o

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=YJFQ20rLK16rise2@mit.edu \
    --to=tytso@mit.edu \
    --cc=adilger@dilger.ca \
    --cc=harshads@google.com \
    --cc=harshadshirwadkar@gmail.com \
    --cc=linux-ext4@vger.kernel.org \
    /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).