mbox series

[v4,00/16] block: sed-opal: support shadow MBR done flag and write

Message ID 1549054223-12220-1-git-send-email-zub@linux.fjfi.cvut.cz (mailing list archive)
Headers show
Series block: sed-opal: support shadow MBR done flag and write | expand

Message

David Kozub Feb. 1, 2019, 8:50 p.m. UTC
This patch series extends SED OPAL support: it adds IOCTL for setting the shadow
MBR done flag which can be useful for unlocking an OPAL disk on boot and it adds
IOCTL for writing to the shadow MBR. Also included are some minor fixes and
improvements.

This series is based on the original work done by Jonas Rabenstein which was
submitted in March 2018.[1]

There is a fork of sed-opal-temp that can use these new IOCTLs.[2] I tested
these on Samsung 840 EVO and 850 EVO drives, on x86-64 and arm64 systems.

The series applies on v5.0-rc4.

I'm resending as v4 as suggested by Scott Bauer.[3]

Changes from v3 to v4:
* added Reviewed-by from Scott, including for the patch 16/16 (details in
[3])

[1] https://lore.kernel.org/lkml/cover.1521482295.git.jonas.rabenstein@studium.uni-erlangen.de/
[2] https://github.com/ghostav/sed-opal-temp
[3] https://lore.kernel.org/lkml/20190127211449.GP21149@hacktheplanet/

David Kozub (8):
  block: sed-opal: fix typos and formatting
  block: sed-opal: close parameter list in cmd_finalize
  block: sed-opal: unify cmd start
  block: sed-opal: unify error handling of responses
  block: sed-opal: reuse response_get_token to decrease code duplication
  block: sed-opal: pass steps via argument rather than via opal_dev
  block: sed-opal: don't repeat opal_discovery0 in each steps array
  block: sed-opal: rename next to execute_steps

Jonas Rabenstein (8):
  block: sed-opal: use correct macro for method length
  block: sed-opal: unify space check in add_token_*
  block: sed-opal: print failed function address
  block: sed-opal: split generation of bytestring header and content
  block: sed-opal: add ioctl for done-mark of shadow mbr
  block: sed-opal: ioctl for writing to shadow mbr
  block: sed-opal: unify retrieval of table columns
  block: sed-opal: check size of shadow mbr

 block/opal_proto.h            |  18 +
 block/sed-opal.c              | 845 +++++++++++++++++-----------------
 include/linux/sed-opal.h      |   2 +
 include/uapi/linux/sed-opal.h |   9 +
 4 files changed, 451 insertions(+), 423 deletions(-)

Comments

David Kozub Feb. 4, 2019, 9:44 a.m. UTC | #1
On Fri, 1 Feb 2019, David Kozub wrote:

> This patch series extends SED OPAL support: it adds IOCTL for setting the shadow
> MBR done flag which can be useful for unlocking an OPAL disk on boot and it adds
> IOCTL for writing to the shadow MBR. Also included are some minor fixes and
> improvements.
>
> This series is based on the original work done by Jonas Rabenstein which was
> submitted in March 2018.[1]
>
> There is a fork of sed-opal-temp that can use these new IOCTLs.[2] I tested
> these on Samsung 840 EVO and 850 EVO drives, on x86-64 and arm64 systems.
>
> The series applies on v5.0-rc4.
>
> I'm resending as v4 as suggested by Scott Bauer.[3]
>
> Changes from v3 to v4:
> * added Reviewed-by from Scott, including for the patch 16/16 (details in
> [3])

So this time really the 16th patch got lost somewhere - while I have 
received it, I could not see it in https://lore.kernel.org/lkml/. So I 
re-sent it now.

Best regards,
David
Christoph Hellwig Feb. 4, 2019, 3:04 p.m. UTC | #2
On Fri, Feb 01, 2019 at 09:50:07PM +0100, David Kozub wrote:
> This patch series extends SED OPAL support: it adds IOCTL for setting the shadow
> MBR done flag which can be useful for unlocking an OPAL disk on boot and it adds
> IOCTL for writing to the shadow MBR. Also included are some minor fixes and
> improvements.

Actually most of it is really useful cleanups and small fixes for the
existing OPAL code.  Any chance you could resend just those bits as
a first series ASAP?  I'll try to spend some time to to review the
actual feature additions in the meantime.

> There is a fork of sed-opal-temp that can use these new IOCTLs.[2] I tested
> these on Samsung 840 EVO and 850 EVO drives, on x86-64 and arm64 systems.

Which brings up another question:  how do we get a properly maintained
version of the sed-opal tool up ASAP?  It's been a bit bitrotting
unfortunately, and the documentation and error handling hasn't been all
that great to start with.  Are we going to find a good home for it?
Util-linux for example?
Scott Bauer Feb. 4, 2019, 3:36 p.m. UTC | #3
On Mon, Feb 04, 2019 at 07:04:15AM -0800, Christoph Hellwig wrote:
> On Fri, Feb 01, 2019 at 09:50:07PM +0100, David Kozub wrote:
> > This patch series extends SED OPAL support: it adds IOCTL for setting the shadow
> > MBR done flag which can be useful for unlocking an OPAL disk on boot and it adds
> > IOCTL for writing to the shadow MBR. Also included are some minor fixes and
> > improvements.
> 
> Actually most of it is really useful cleanups and small fixes for the
> existing OPAL code.  Any chance you could resend just those bits as
> a first series ASAP?  I'll try to spend some time to to review the
> actual feature additions in the meantime.
> 
> > There is a fork of sed-opal-temp that can use these new IOCTLs.[2] I tested
> > these on Samsung 840 EVO and 850 EVO drives, on x86-64 and arm64 systems.
> 
> Which brings up another question:  how do we get a properly maintained
> version of the sed-opal tool up ASAP?  It's been a bit bitrotting
> unfortunately, and the documentation and error handling hasn't been all
> that great to start with.  Are we going to find a good home for it?
> Util-linux for example?

I would also like to get it off my github and into a proper location as well.

Christoph, do you know off hand how we would get it in util-linux?
Christoph Hellwig Feb. 4, 2019, 3:44 p.m. UTC | #4
On Mon, Feb 04, 2019 at 10:36:10AM -0500, Scott Bauer wrote:
> > Which brings up another question:  how do we get a properly maintained
> > version of the sed-opal tool up ASAP?  It's been a bit bitrotting
> > unfortunately, and the documentation and error handling hasn't been all
> > that great to start with.  Are we going to find a good home for it?
> > Util-linux for example?
> 
> I would also like to get it off my github and into a proper location as well.
> 
> Christoph, do you know off hand how we would get it in util-linux?

Maybe just ask util-linux@vger.kernel.org if they are interested?
David Kozub Feb. 4, 2019, 11:06 p.m. UTC | #5
On Mon, 4 Feb 2019, Christoph Hellwig wrote:

> On Fri, Feb 01, 2019 at 09:50:07PM +0100, David Kozub wrote:
>> This patch series extends SED OPAL support: it adds IOCTL for setting the shadow
>> MBR done flag which can be useful for unlocking an OPAL disk on boot and it adds
>> IOCTL for writing to the shadow MBR. Also included are some minor fixes and
>> improvements.
>
> Actually most of it is really useful cleanups and small fixes for the
> existing OPAL code.  Any chance you could resend just those bits as
> a first series ASAP?  I'll try to spend some time to to review the
> actual feature additions in the meantime.

OK, I'll try to separate these into two sets.

This will unfortunately trigger some changes (conflict resolving - e.g. if 
I move the last two patches in the current series forward, in front of the 
patches with new functionality). What is the proper procedure w.r.t. 
Reviewed-by tags which were already given? Common sense would make me keep 
them for trivial changes and remove them if the patch should be 
re-reviewed. Is that correct?

>> There is a fork of sed-opal-temp that can use these new IOCTLs.[2] I tested
>> these on Samsung 840 EVO and 850 EVO drives, on x86-64 and arm64 systems.
>
> Which brings up another question:  how do we get a properly maintained
> version of the sed-opal tool up ASAP?  It's been a bit bitrotting
> unfortunately, and the documentation and error handling hasn't been all
> that great to start with.  Are we going to find a good home for it?
> Util-linux for example?

FWIW I also hacked together a tool to cover my usecase: 
https://gitlab.com/zub2/opalctl

The selling point is that it can handle passwords the same way that 
sedutil (https://github.com/Drive-Trust-Alliance/sedutil) does.

Best regards,
David
Christoph Hellwig Feb. 5, 2019, 6:57 a.m. UTC | #6
On Tue, Feb 05, 2019 at 12:06:54AM +0100, David Kozub wrote:
> This will unfortunately trigger some changes (conflict resolving - e.g. if I
> move the last two patches in the current series forward, in front of the
> patches with new functionality). What is the proper procedure w.r.t.
> Reviewed-by tags which were already given? Common sense would make me keep
> them for trivial changes and remove them if the patch should be re-reviewed.
> Is that correct?

I don't think there is a general rule.  As long as the change is trivial
I keep them personally, otherwise I drop them.

E.g. if you just remove a few hunk because of new code that isn't
present now I'd keep them, if there are more complex changes to the code
flow I would consider dropping them.

> FWIW I also hacked together a tool to cover my usecase:
> https://gitlab.com/zub2/opalctl
> 
> The selling point is that it can handle passwords the same way that sedutil
> (https://github.com/Drive-Trust-Alliance/sedutil) does.

That actually is pretty nice, as it allows people to migrate over
(not that I've heard of a whole lot of usage of sedutil, but being
compatible to some extent is always nice)