linux-kernel - Re: [PATCH] libosd: Remove ignored _

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]

Message-ID: <65187f1a-4e67-3d96-ea97-e37c8bf87e48@electrozaur.com>
Date:   Thu, 1 Nov 2018 03:15:56 +0200
From:   Boaz Harrosh <ooo@...ctrozaur.com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Nathan Chancellor <natechancellor@...il.com>
Cc:     bvanassche@....org,
        "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        linux-scsi@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        hch@...radead.org
Subject: Re: [PATCH] libosd: Remove ignored __weak attribute

On 26/10/18 20:54, Nick Desaulniers wrote:
<>
> 
> It's hard to say without knowing the original intent of the code.
>>>From the variable's identifier and fact that it's global, I *assume*
> that we want only 1 struct osd_obj_id which is the root, hence the
> identifier `osd_root_object`.  It has 4 references in the entire
> kernel; it doesn't make sense to my why those references would want to
> be referring to two different instances of `osd_root_object`.  Maybe
> the maintainers can clarify if 2 instances is the intent?
> 
> Further complicated is the use of the __weak attribute AND the
> compiler flag -fno-common (which the kernel sets in the top level
> Makefile).  Also, it seems that ODR is a C++ concept; it's not clear
> to me if there's semantic differences with C or not (I don't think so
> in this case, but I've learned not to bet on subtle semantic
> differences between the languages not existing).
> 
> __attribute__((weak)) AND static on a global variable declared in a
> header raises many red flags to me.  Was weak added to work around an
> ODR link error?
> 
> If creating one instance of this variable is a functional change, I
> can't help but suspect the original code was wrong.  But maybe Bart,
> Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> OSDv1 Headers").
> 

Sorry for the late response. Was off line for a bit.

The original patch and all its permutations are all correct
the definition of osd_root_object is just a fancy type cast of
couple of zeros. IE a couple of zeroes with the right type. So they
can be simply compared to retuned and sent line content. So it does
not matter at all what change is accepted.

I'm so sorry for your trouble and test development. It could all
be saved if I was noticing this thread.

I will ACK again the original simple V2 patch.

Thanks
Boaz

<>