linux-kernel - Re: [PATCH 06/10] staging: r8188eu: remove DBG_88E calls from os_dep/ioctl

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: <b5bbe488-ac43-fd89-7c65-36bfa9c903a6@gmail.com>
Date:   Wed, 26 Jan 2022 13:26:08 +0300
From:   Pavel Skripkin <paskripkin@...il.com>
To:     Phillip Potter <phil@...lpotter.co.uk>
Cc:     gregkh@...uxfoundation.org, dan.carpenter@...cle.com,
        Larry.Finger@...inger.net, straube.linux@...il.com,
        martin@...ser.cx, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/10] staging: r8188eu: remove DBG_88E calls from
 os_dep/ioctl_linux.c

Hi Phillip,

On 1/26/22 04:13, Phillip Potter wrote:

[snip]

}
>> 
>> And here you also removes the reads. I guess, some kind of magic pattern is
>> used
>> 
> 
> So these calls are macro arguments, they would never be executed under
> normal circumstances anyway, unless the rtw_debug kernel module was
> passed in as 5 or more - it is 1 by default. The DBG_88E macro would
> expand during preprocessing phase to (for example):
> 
> do {
> 	if (5 <= GlobalDebugLevel)
> 		pr_info("R8188EU: " "dbg(0x450) = 0x%x\n", rtw_read32(padapter, 0x450));
> } while (0)
> 
> As this is never executed under normal circumstances anyway, I would say
> calls like these are therefore safe to remove. Happy to be convinced
> though :-) Many thanks.
> 

I see your point, thanks for explanation.

Well, in this case, you may left all reads, that are executed during 
normal lifetime of a driver. We know, that there is at least 1 place, 
where read() call removal can break things. Might be there are couple of 
other places we don't know about.

IMHO the best thing you can do is to leave these reads and leave a 
comment like "hey, please remove me and test". One day useless reads 
should be anyway removed, since ideally rtw_read family must get 
__must_check annotation + normal error handling.

Thanks :)

With regards,
Pavel Skripkin