linux-kernel - Re: [PATCH v2] staging: r8188eu: Use kzalloc() with GFP

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: <11283844.I1tDBM3C2B@localhost.localdomain>
Date:   Mon, 01 Nov 2021 17:30:47 +0100
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Phillip Potter <phil@...lpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Larry Finger <Larry.Finger@...inger.net>
Subject: Re: [PATCH v2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context

On Monday, November 1, 2021 4:11:26 PM CET Larry Finger wrote:
> On 11/1/21 09:27, Fabio M. De Francesco wrote:
> > Use the GFP_ATOMIC flag of kzalloc() with two memory allocation in
> > report_del_sta_event(). This function is called while holding spinlocks,
> > therefore it is not allowed to sleep. With the GFP_ATOMIC type flag, the
> > allocation is high priority and must not sleep.
> > 
> > This issue is detected by Smatch which emits the following warning:
> > "drivers/staging/r8188eu/core/rtw_mlme_ext.c:6848 report_del_sta_event()
> > warn: sleeping in atomic context".
> >
> > []
> >
> I am happy that you caught the error before it destroyed every instance of 
> r8188eu.

I don't think so, since we have run this driver with no problems at all :)

SAC bugs can potentially cause serious system hangs at runtime, but they do 
not always cause problems in real execution as you have noticed here with 
this driver. We have used and tested it hundreds of times with no problems.

> Incidentally, I disagree with checkpatch in that I think that 
> sizeof(struct foo) is more descriptive than sizeof(*bar). 

I agree with you in full, but I felt that I had to change it just because of 
the warning output by that tool. I don't like to have my patches discarded 
because they don't fix checkpatch warnings or introduce new ones. 

> If I wanted to check 
> the resulting value of the sizeof(), the second form requires an additional 
> step. It probably does not matter much to the compiler, but when I have to 
do it 
> manually, the extra effort is not negligible.
> 
> Even though I disagree with the philosophy,
> 
> Acked-by: Larry Finger <Larry.Finger@...inger.net>
> 

Thanks for your "Acked-by" tag.

Fabio