cryptsetup: add support for sector-size= option (#8881) #9936

xnox · 2018-08-24T17:23:53Z

Bug-Ubuntu: https://launchpad.net/bugs/1776626

xnox · 2018-08-24T17:24:41Z

Validated that the option is accepted, and that dmsetup table shows that it is active with a 4096 size.

Will work on adding the identical option to cryptsetup package in debian/ubuntu too, to keep the two implementations in sync.

hbrueckner · 2018-08-27T12:10:36Z

Hi,
below you can find some review feedback on your PR diff as I also started to have a look at it.

The code looks fine and should work for cryptsetup >= 2.x. On the other side, I think it will not compile on older cryptsetup (libcryptsetup) version because the sector_size is a new member in struct crypt_params_plain.

To distinguish whether the sector_size member is present, I think it is sufficient to check for CRYPT_LUKS2 as 4K sector size support has been added to cryptsetup as of 2.x.

So for example, you might change as follows:

+        } else if ((val = startswith(option, "sector-size="))) {
+#ifdef CRYPT_LUKS2
+                r = safe_atou(val, &arg_sector_size);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to parse %s: %m", option);
+                /* Do the other checks here ... */
+#else
+                log_error("This version of cryptsetup does not support different sector sizes; refusing.");
+                return -EINVAL;
+#endif

/* [..] */

                 struct crypt_params_plain params = {
                         .offset = arg_offset,
                         .skip = arg_skip,
+#ifdef CRYPT_LUKS2
+                        .sector_size = arg_sector_size,
+#endif
                 };

keszybz · 2018-08-27T12:26:17Z

man/crypttab.xml

+      <varlistentry>
+        <term><option>sector-size=</option></term>
+
+        <listitem><para>Specifies the sector size in bits. See


keszybz · 2018-08-27T12:28:36Z

src/cryptsetup/cryptsetup.c

+                }
+
+                if (arg_sector_size < CRYPT_SECTOR_SIZE || arg_sector_size > CRYPT_MAX_SECTOR_SIZE) {
+                        log_error("sector-size= is outside of %u and %u.", CRYPT_SECTOR_SIZE, CRYPT_MAX_SECTOR_SIZE);


", ignoring"

xnox · 2018-08-28T00:00:54Z

@hbrueckner

I'm not sure I care at all compiling against old cryptsetup. It has been out for a while now, I would just bump the requirement to >= 2.0.0. E.g. meson was bumped to 0.46 as well - albeit that is not a runtime dep. I guess it depends on what we need to support or want to require.

Feel a bit like wasting my time now, if there were patches already available. Were you not able to share them earlier when LTC requested this feature in Ubuntu? 😔

hbrueckner · 2018-08-28T07:33:28Z

@xnox

I agree on increasing the cryptsetup version to >= 2.0.0 to avoid the #ifdefs. My intention was to keep some backward compatibility to older libcryptsetup versions at build time. But for upstream this is indeed not an issue.

Regarding the LTC request, I was not really aware of that. I started to look at this issue when Ingo created the systemd issue. Honestly, I did not have the time to test the patch at all, that's why I am glad that you have shard your version.

Looking at your change again, I have one minor comment (will follow separately), otherwise I am fine with your version with the build dependency updated.

Many thanks.

hbrueckner · 2018-08-28T07:34:25Z

man/crypttab.xml

+      <varlistentry>
+        <term><option>sector-size=</option></term>
+
+        <listitem><para>Specifies the sector size in bits. See


Could you make this a bit more clear. For example:

Specifies the sector size in bytes for use with disk encryption in plain mode.

We should not really copy the cryptsetup manpage here, as the cryptsetup (8) reference has the details about the option, including caveats etc.

That's ok.. but you commit just changes for plain mode. So it would be good prevent confusing and yet another iteration of man page look-ups. Of course, the full description can be found in the cryptsetup man-page and it is important to add a reference.

yuwata · 2018-08-28T07:51:41Z

Hmm, as already mentioned by @hbrueckner, crypt_params_plain.sector_size is added in v2. The semaphoreci seems to use v1.x, and it fails... I tentatively tag this 'ci-fails/needs-rework'.

xnox · 2018-08-28T09:14:03Z

@yuwata looking at semaphore setup, it still uses xenial as a base, and then pulls more things from pitti's ppa and then adds artful..... and artful is EOL btw. IMHO semaphore CI should be moved to bionic as well. That will get one newer things / closer to what people integrate new upstream releases on.

How can I see full configs for semaphore ci and propose changes to it? i'm not that familiar with it - any pointers would be appreciated.

yuwata · 2018-08-28T09:36:31Z

How can I see full configs for semaphore ci and propose changes to it? i'm not that familiar with it - any pointers would be appreciated.

I guess @keszybz can help you.

Anyway, if you want to drop supporting v1.x, then please also update meson.build and README.

Bug-Ubuntu: https://launchpad.net/bugs/1776626

xnox · 2018-08-28T12:57:27Z

@yuwata added conditionals for now; and will do the semaphore bump separately; and then the drop of the support for old libcryptsetup

@keszybz comments addressed

all CI passes now

xnox · 2018-08-29T14:11:00Z

How can I check what is wrong with the Fedora Rawhide build?

yuwata · 2018-08-29T14:36:06Z

LGTM.

How can I check what is wrong with the Fedora Rawhide build?

@xnox Sometimes it fails with 'internal error' and we cannot get the reason... Please just ignore it.

evverx · 2018-08-30T05:35:59Z

src/cryptsetup/cryptsetup.c


 static const char *arg_type = NULL; /* ANY_LUKS, CRYPT_LUKS1, CRYPT_LUKS2, CRYPT_TCRYPT or CRYPT_PLAIN */
 static char *arg_cipher = NULL;
 static unsigned arg_key_size = 0;
+static unsigned arg_sector_size = CRYPT_SECTOR_SIZE;


This line should probably be enclosed in "#if/#endif" so as not to get

[649/1610] Generating sanitize-address-fuzzers with a custom command. ../src/cryptsetup/cryptsetup.c:34:17: warning: ‘arg_sector_size’ defined but not used [-Wunused-variable] static unsigned arg_sector_size = CRYPT_SECTOR_SIZE; ^

@xnox could you enclose the definition of arg_sector_size in #if/#endif? The warning is still there.

I've created PR ~~#9936~~ #9990.

ah, thanks, I missed this comment entirely.

evverx · 2018-08-30T05:45:41Z

Does anybody know when the issue the link points to will be publicly available?

Follow-up for systemd#9936.

Follow-up for #9936.

xnox · 2018-09-10T15:47:03Z

@evverx it is publically available.

@yuwata is that follow up to avoid "unused variable" warning, or some such?

yuwata · 2018-09-10T16:28:34Z

@xnox #9990 is created and already merged.

xnox · 2018-09-10T18:51:54Z

@yuwata yes, it is merged, my question is why it is needed?

evverx · 2018-09-10T19:46:21Z

@xnox because it's usually a good idea to write code that either doesn't compile with older libraries or, if it has been decided that it should be backwards compatible, compiles without any warnings. At https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options some flags that people might use to make sure that the code they're building isn't problematic are documented, of which the most interesting in the context of this PR is -Werror. If you're wondering what warning I'm talking about you could take a look at #9936 (comment). I hope it helps.

yuwata · 2018-09-11T00:33:54Z

@evverx Thank you for the explanation :-)

Bug-Ubuntu: https://launchpad.net/bugs/1776626 Closes systemd#8881. (cherry picked from commit a9fc640) [fbui: adjust context] [fbui: fixes fate#325634]

…md#9990) Follow-up for systemd#9936. (cherry picked from commit 645461f)

Bug-Ubuntu: https://launchpad.net/bugs/1776626 Closes systemd#8881. (cherry picked from commit a9fc640) [fbui: adjust context] [fbui: fixes fate#325697]

…md#9990) Follow-up for systemd#9936. (cherry picked from commit 645461f)

xnox mentioned this pull request Aug 24, 2018

Add sector-size parameter to crypttab for volumes encrypted with plain mode #8881

Closed

keszybz requested changes Aug 27, 2018

View reviewed changes

keszybz added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks cryptsetup labels Aug 27, 2018

hbrueckner reviewed Aug 28, 2018

View reviewed changes

yuwata added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Aug 28, 2018

cryptsetup: add support for sector-size= option (systemd#8881)

9a63ee5

Bug-Ubuntu: https://launchpad.net/bugs/1776626

xnox force-pushed the sector-size branch from f789783 to 9a63ee5 Compare August 28, 2018 10:40

yuwata removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Aug 29, 2018

yuwata merged commit a9fc640 into systemd:master Aug 29, 2018

evverx reviewed Aug 30, 2018

View reviewed changes

yuwata added a commit to yuwata/systemd that referenced this pull request Sep 1, 2018

cryptsetup: do not define arg_sector_size if libgcrypt is v1.x

2a737a8

Follow-up for systemd#9936.

yuwata mentioned this pull request Sep 1, 2018

cryptsetup: do not define arg_sector_size if libgcrypt is v1.x #9990

Merged

yuwata added a commit that referenced this pull request Sep 1, 2018

cryptsetup: do not define arg_sector_size if libgcrypt is v1.x (#9990)

645461f

Follow-up for #9936.

Werkov pushed a commit to Werkov/systemd that referenced this pull request Nov 27, 2018

cryptsetup: do not define arg_sector_size if libgcrypt is v1.x (syste…

bd95d2c

…md#9990) Follow-up for systemd#9936. (cherry picked from commit 645461f)

Werkov pushed a commit to Werkov/systemd that referenced this pull request Jun 3, 2020

cryptsetup: do not define arg_sector_size if libgcrypt is v1.x (syste…

8a77eed

…md#9990) Follow-up for systemd#9936. (cherry picked from commit 645461f)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cryptsetup: add support for sector-size= option (#8881) #9936

cryptsetup: add support for sector-size= option (#8881) #9936

xnox commented Aug 24, 2018

xnox commented Aug 24, 2018

hbrueckner commented Aug 27, 2018 •

edited by keszybz

Loading

keszybz Aug 27, 2018

xnox Aug 28, 2018

keszybz Aug 27, 2018

xnox Aug 28, 2018

xnox commented Aug 28, 2018

hbrueckner commented Aug 28, 2018

hbrueckner Aug 28, 2018

xnox Aug 28, 2018

hbrueckner Aug 28, 2018

yuwata commented Aug 28, 2018

xnox commented Aug 28, 2018

yuwata commented Aug 28, 2018

xnox commented Aug 28, 2018

xnox commented Aug 29, 2018

yuwata commented Aug 29, 2018

evverx Aug 30, 2018

evverx Sep 1, 2018

yuwata Sep 1, 2018 •

edited

Loading

xnox Sep 11, 2018

evverx commented Aug 30, 2018

xnox commented Sep 10, 2018

yuwata commented Sep 10, 2018

xnox commented Sep 10, 2018

evverx commented Sep 10, 2018

yuwata commented Sep 11, 2018

cryptsetup: add support for sector-size= option (#8881) #9936

cryptsetup: add support for sector-size= option (#8881) #9936

Conversation

xnox commented Aug 24, 2018

xnox commented Aug 24, 2018

hbrueckner commented Aug 27, 2018 • edited by keszybz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xnox commented Aug 28, 2018

hbrueckner commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwata commented Aug 28, 2018

xnox commented Aug 28, 2018

yuwata commented Aug 28, 2018

xnox commented Aug 28, 2018

xnox commented Aug 29, 2018

yuwata commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwata Sep 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evverx commented Aug 30, 2018

xnox commented Sep 10, 2018

yuwata commented Sep 10, 2018

xnox commented Sep 10, 2018

evverx commented Sep 10, 2018

yuwata commented Sep 11, 2018

hbrueckner commented Aug 27, 2018 •

edited by keszybz

Loading

yuwata Sep 1, 2018 •

edited

Loading