Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3e491cfe02590cf5fce49851cf8fa040e0a4c8f6.camel@redhat.com>
Subject: Re: [PATCH 3/6] composefs: Add descriptor parsing code
From:   Alexander Larsson <alexl@redhat.com>
To:     Brian Masney <bmasney@redhat.com>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        gscrivan@redhat.com
Date:   Tue, 10 Jan 2023 17:33:12 +0100
In-Reply-To: <Y7cszNNvHHUef2qO@x1>
References: <cover.1669631086.git.alexl@redhat.com>
         <1c4c49fac5bb6406a8cb55ca71f8060703aa63f6.1669631086.git.alexl@redhat.com>
         <Y7cszNNvHHUef2qO@x1>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.2 (3.46.2-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-01-05 at 15:02 -0500, Brian Masney wrote:
> On Mon, Nov 28, 2022 at 12:16:59PM +0100, Alexander Larsson wrote:
> > This adds the code to load and decode the filesystem descriptor
> > file
> > format.
> >=20
> >=20
> > +#define CFS_N_PRELOAD_DIR_CHUNKS 4
>=20
> From looking through the code it appears that this is actually the
> maximum number of chunks. Should this be renamed from PRELOAD to MAX?
>=20

No, this is the number of directory chunks we statically pre-load while
loading the inode. If there are more (i.e. for larger directories) we
load chunks dynamically as needed during the directory operations.

>=20
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0header->magic =3D cfs_u32_fr=
om_file(header->magic);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0header->data_offset =3D cfs_=
u64_from_file(header-
> > >data_offset);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0header->root_inode =3D cfs_u=
64_from_file(header->root_inode);
>=20
> Should the cpu to little endian conversion occur in cfs_read_data()?

cfs_read_data() reads just a block of opaque data. It can't know which
parts make up individual values to convert.

> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (header->magic !=3D CFS_M=
AGIC ||
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 header->d=
ata_offset > ctx.descriptor_len ||
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sizeof(st=
ruct cfs_header_s) + header->root_inode >
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ctx.descriptor_len) {
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0res =3D -EINVAL;
>=20
> Should this be -EFSCORRUPTED?
>=20

I don't think so. I can see the argument for it, but at this point we
just looked at a file based on a mount argument, and it seems
completely wrong (not just corrupt in the middle). Reporting EINVAL in
the mount syscall for "invalid argument" seems more right to me.


> >=20
> > +static void *cfs_get_vdata_buf(struct cfs_context_s *ctx, u64
> > offset, u32 len,
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct cfs_buf *buf)
> > +{
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (offset > ctx->descriptor=
_len - ctx->header.data_offset)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return ERR_PTR(-EINVAL);
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (len > ctx->descriptor_le=
n - ctx->header.data_offset -
> > offset)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return ERR_PTR(-EINVAL);
>=20
> It appears that these same checks are already done in cfs_get_buf().
>=20

Not quite. It is true that we check in cfs_get_buf() that the arguments
are not completely outside the file. However, this part checks that the
offset is specifically within the vdata part of the file. In
particular, if we rely on the checks in cfs_get_buf() we could get
confused if the below data_offset + offset wrapped.

> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return cfs_get_buf(ctx, ctx-=
>header.data_offset + offset,
> > len, buf);
> > +}


>=20
>=20
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ino->flags =3D cfs_read_u32(=
&data);
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0inode_size =3D cfs_inode_enc=
oded_size(ino->flags);
>=20
> Should CFS_INODE_FLAGS_DIGEST_FROM_PAYLOAD also be accounted for in
> cfs_inode_encoded_size()?

No, that flag just means that the already existing payload (which
contains the pathname for the backing data) should be used as the
source for the digest (to avoid storing it twice). It doesn't take up
any extra space otherwise.

> Also, cfs_inode_encoded_size() is only used here so can be brought
> into this file.
>=20

I see this as sort of part of the filesystem on-disk format, so I
rather have it in the cfs.h header with the rest of the fs definition.

>=20
> > +static bool cfs_validate_filename(const char *name, size_t
> > name_len)
> > +{
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (name_len =3D=3D 0)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return false;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (name_len =3D=3D 1 && nam=
e[0] =3D=3D '.')
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return false;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (name_len =3D=3D 2 && nam=
e[0] =3D=3D '.' && name[1] =3D=3D '.')
>=20
> Can strcmp() be used here?
>=20

name is not zero-terminated, so I don't think so. At least not in any
natural way.

> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0inode_data->has_digest =3D r=
et !=3D 0;
>=20
> Can you do 'has_digest =3D inode_data->digest !=3D NULL;' to get rid of
> the need for return 1 in cfs_get_digest().

No, because ->digest is an in-line array, not a pointer.

> > +static inline int memcmp2(const void *a, const size_t a_size,
> > const void *b,
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
 size_t b_size)
> > +{
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0size_t common_size =3D min(a=
_size, b_size);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int res;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0res =3D memcmp(a, b, common_=
size);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (res !=3D 0 || a_size =3D=
=3D b_size)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0return res;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return a_size < b_size ? -1 =
: 1;
>=20
> This function appears to be used only in one place below. It doesn't
> seem like it matters for the common_size. Can this just be dropped
> and use memcmp()?

Not sure what you mean by this. This is used as a sort/order function,
not just as an "is-the-same" check, so we have to report e.g. that
"prefixXYZ" is after "prefix". In other words, this is essentially
strcmp(), but the strings are not zero terminated.

--=20
=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D=
-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-
=3D-=3D-=3D
 Alexander Larsson                                            Red Hat,
Inc=20
       alexl@redhat.com            alexander.larsson@gmail.com=20
He's a deeply religious Amish hairdresser who hides his scarred face=20
behind a mask. She's a mentally unstable gypsy bodyguard in the witness
protection program. They fight crime!=20