lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 3 Nov 2022 01:22:59 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Elliot Berman <quic_eberman@...cinc.com>
Cc:     Jassi Brar <jassisinghbrar@...il.com>,
        Bjorn Andersson <quic_bjorande@...cinc.com>,
        Murali Nalajala <quic_mnalajal@...cinc.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
        Carl van Schaik <quic_cvanscha@...cinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
        Andy Gross <agross@...nel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        linux-arm-kernel@...ts.infradead.org,
        Mark Rutland <mark.rutland@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Sudeep Holla <sudeep.holla@....com>,
        Marc Zyngier <maz@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jonathan Corbet <corbet@....net>,
        Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Arnd Bergmann <arnd@...db.de>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Amol Maheshwari <amahesh@....qualcomm.com>,
        Kalle Valo <kvalo@...nel.org>, devicetree@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 10/21] gunyah: rsc_mgr: Add resource manager RPC core

On Wed, Nov 02, 2022 at 11:04:45AM -0700, Elliot Berman wrote:
> > > > > +/* Resource Manager Header */
> > > > > +struct gh_rm_rpc_hdr {
> > > > > +	u8 version : 4, hdr_words : 4;
> > > > > +	u8 type : 2, fragments : 6;
> > > > 
> > > > Ick, that's hard to read.  One variable per line please?
> > > 
> > > Ack.
> > > 
> > > > And why the bit packed stuff?  Are you sure this is the way to do this?
> > > > Why not use a bitmask instead?
> > > > 
> > > 
> > > I felt bit packed implementation is cleaner and easier to map to
> > > understanding what the fields are used for.
> > 
> > Ah, so this isn't what is on the "wire", then don't use a bitfield like
> > this, use a real variable and that will be faster and simpler to
> > understand.
> > 
> 
> This is what's on the "wire". Whether I use bitfield or bit packed would be
> functionally the same and is just a cosmetic change IMO.

Ah, that wasn't obvious at all.

Usually using bitfields like this for "wire" protocols is not a good
idea (endian issues and all of that.)  Please use a bitmask instead, as
that way you know exactly what is happening, and the compiler can
usually generate much better code overall.

And as this is on the wire, please specify the endian values, _AND_ use
the proper kernel types for stuff that goes between user/kernel or
kernel/hardware, as you are not doing that here.

thanks,

greg k-h

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ