linux-kernel - Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

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: <94770fc6-7d72-8283-5858-786685620d5c@linaro.org>
Date:   Tue, 12 Sep 2023 11:54:17 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Mukesh Ojha <quic_mojha@...cinc.com>, corbet@....net,
        agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org, keescook@...omium.org, tony.luck@...el.com,
        gpiccoli@...lia.com, mathieu.poirier@...aro.org,
        catalin.marinas@....com, will@...nel.org, linus.walleij@...aro.org,
        andy.shevchenko@...il.com, vigneshr@...com, nm@...com,
        matthias.bgg@...il.com, kgene@...nel.org, alim.akhtar@...sung.com,
        bmasney@...hat.com, quic_tsoni@...cinc.com
Cc:     linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-hardening@...r.kernel.org,
        linux-remoteproc@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
        linux-mediatek@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, kernel@...cinc.com
Subject: Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel
 driver

On 12/09/2023 11:26, Mukesh Ojha wrote:
>>
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	mutex_init(&md->md_lock);
>>> +	ret = qcom_apss_md_table_init(md, &mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
>>> +	if (ret) {
>>> +		dev_err(md->dev, "apss minidump initialization failed: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* First entry would be ELF header */
>>> +	ret = qcom_md_add_elfheader(md);
>>> +	if (ret) {
>>> +		dev_err(md->dev, "Failed to add elf header: %d\n", ret);
>>> +		memset(md->apss_data->md_ss_toc, 0, sizeof(struct minidump_subsystem));
>>
>> Why do you need it?
> 
> Earlier, i got comment about clearing the SS TOC(subsystem table of 
> content) which is shared with other SS and it will have stale values.

OK, but then the entire code is poorly readable. First, any cleanup of
qcom_apss_md_table_init() should be named similarly, e.g.
qcom_apss_md_table_clean() or qcom_apss_md_table_exit() or whatever
seems feasible.

Second, shouldn't writing to shared memory be the last step? Step which
cannot fail and there is no cleanup afterwards (like
platform_set_drvdata)? I don't enjoy looking at this interface...



Best regards,
Krzysztof