Linux-FSCrypt Archive on lore.kernel.org
 help / color / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: Jes Sorensen <jes.sorensen@gmail.com>
Cc: linux-fscrypt@vger.kernel.org, jsorensen@fb.com, kernel-team@fb.com
Subject: Re: [PATCH 2/3] Introduce libfsverity
Date: Thu, 21 May 2020 09:59:41 -0700
Message-ID: <20200521165941.GB12790@gmail.com> (raw)
In-Reply-To: <2b2a2747-93e7-3a86-5d7f-86ec9fd5b207@gmail.com>

On Thu, May 21, 2020 at 12:45:57PM -0400, Jes Sorensen wrote:
> >> My biggest objection is the export of kernel datatypes to userland and I
> >> really don't think using u32/u16/u8 internally in the library adds any
> >> value. I had explicitly converted it to uint32_t/uint16_t/uint8_t in my
> >> version.
> > 
> > I prefer u64/u32/u16/u8 since they're shorter and easier to read, and it's the
> > same convention that is used in the kernel code (which is where the other half
> > of fs-verity is).
> 
> I like them too, but I tend to live in kernel space.
> 
> > Note that these types aren't "exported" to or from anywhere but rather are just
> > typedefs in common/common_defs.h.  It's just a particular convention.
> > 
> > Also, fsverity-utils is already using this convention prior to this patchset.
> > If we did decide to change it, then we should change it in all the code, not
> > just in certain places.
> 
> I thought I did it everywhere in my patch set?

No, they were still left in various places.

> 
> >> I also wonder if we should introduce an
> >> libfsverity_get_digest_size(alg_nr) function? It would be useful for a
> >> caller trying to allocate buffers to store them in, to be able to do
> >> this prior to calculating the first digest.
> > 
> > That already exists; it's called libfsverity_digest_size().
> > 
> > Would it be clearer if we renamed:
> > 
> > 	libfsverity_digest_size() => libfsverity_get_digest_size()
> > 	libfsverity_hash_name() => libfsverity_get_hash_name()
> 
> Oh I missed you added that. Probably a good idea to rename them for
> consistency.

libfsverity_digest_size() was actually in your patchset too.

I'll add the "get" to the names so that all function names start with a verb.

> >>> +static void *xmalloc(size_t size)
> >>> +{
> >>> +	void *p = malloc(size);
> >>> +
> >>> +	if (!p)
> >>> +		libfsverity_error_msg("out of memory");
> >>> +	return p;
> >>> +}
> >>> +
> >>> +void *libfsverity_zalloc(size_t size)
> >>> +{
> >>> +	void *p = xmalloc(size);
> >>> +
> >>> +	if (!p)
> >>> +		return NULL;
> >>> +	return memset(p, 0, size);
> >>> +}
> >>
> >> I suggest we get rid of xmalloc() and libfsverity_zalloc(). libc
> >> provides perfectly good malloc() and calloc() functions, and printing an
> >> out of memory error in a generic location doesn't tell us where the
> >> error happened. If anything those error messages should be printed by
> >> the calling function IMO.
> >>
> > 
> > Maybe.  I'm not sure knowing the specific allocation sites would be useful
> > enough to make all the callers handle printing the error message (which is
> > easily forgotten about).  We could also add the allocation size that failed to
> > the message here.
> 
> My point is mostly at this point, this just adds code obfuscation rather
> than adding real value. I can see how it was useful during initial
> development.

It's helpful to eliminate the need for callers to print the error message.

We also still need libfsverity_memdup() anyway, unless we hard-code
malloc() + memcpy().

I also had in mind that we'd follow the (increasingly recommended) practice of
initializing all heap memory.  This can be done by only providing allocation
functions that initialize memory.

I'll think about it.

- Eric

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-15  4:10 [PATCH 0/3] fsverity-utils: introduce libfsverity Eric Biggers
2020-05-15  4:10 ` [PATCH 1/3] Split up cmd_sign.c Eric Biggers
2020-05-21 15:26   ` Jes Sorensen
2020-05-15  4:10 ` [PATCH 2/3] Introduce libfsverity Eric Biggers
2020-05-21 15:24   ` Jes Sorensen
2020-05-21 16:08     ` Eric Biggers
2020-05-21 16:45       ` Jes Sorensen
2020-05-21 16:59         ` Eric Biggers [this message]
2020-05-21 17:13           ` Jes Sorensen
2020-05-15  4:10 ` [PATCH 3/3] Add some basic test programs for libfsverity Eric Biggers
2020-05-21 15:29   ` Jes Sorensen
2020-05-15 20:50 ` [PATCH 0/3] fsverity-utils: introduce libfsverity Jes Sorensen
2020-05-20  3:06   ` Eric Biggers
2020-05-20 13:26     ` Jes Sorensen

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=20200521165941.GB12790@gmail.com \
    --to=ebiggers@kernel.org \
    --cc=jes.sorensen@gmail.com \
    --cc=jsorensen@fb.com \
    --cc=kernel-team@fb.com \
    --cc=linux-fscrypt@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

Linux-FSCrypt Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lkml.kernel.org/linux-fscrypt/0 linux-fscrypt/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fscrypt linux-fscrypt/ https://lkml.kernel.org/linux-fscrypt \
		linux-fscrypt@vger.kernel.org
	public-inbox-index linux-fscrypt

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fscrypt


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git