Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 25 Apr 2022 21:48:23 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Vihas Makwana <makvihas@gmail.com>
Cc:     Pavel Skripkin <paskripkin@gmail.com>,
        Larry Finger <Larry.Finger@lwfinger.net>,
        Phillip Potter <phil@philpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Michael Straube <straube.linux@gmail.com>,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] staging: r8188eu: fix null check in
 _rtw_free_mlme_priv
Message-ID: <20220425184822.GR2462@kadam>
References: <20220424163132.37007-1-makvihas@gmail.com>
 <20220424163132.37007-4-makvihas@gmail.com>
 <2e4fa41d-cd06-75e4-17ac-120127fe9b2a@gmail.com>
 <CAH1kMwTT7kEB5O6ZvCWEJk3RGzkRek_fzaNPUnUpFFKq_fhGvQ@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAH1kMwTT7kEB5O6ZvCWEJk3RGzkRek_fzaNPUnUpFFKq_fhGvQ@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?AZsRE5mVD3fKuwegVyU0nKE8phrcgqihv1xs6Yf+rLwdk3VQG11+Kuja3nhf?=
 =?us-ascii?Q?f50WupQpgMPHmSYcXeSofMgw+J1gpunMec8svIEe+geczcx3CAHcWe+3ln7g?=
 =?us-ascii?Q?Ky9VHXEgWvKGeBZyY/4k+Dq8VLxkSdo0S27+DISu7ZH0iW5EsMxoY5PXrSjr?=
 =?us-ascii?Q?fuxeYL+za+5msA0xJh0WYssqOOdV7UAaDYBoqtBJ5znD5BGEDqq5xgllwSJw?=
 =?us-ascii?Q?9ZqpmwHBfE0EcsVoWr+vGhdyWqvMQJ0qA4O/1WboJziAEM/L7WeBNj4HLSMq?=
 =?us-ascii?Q?kAVGr7D7duKU78EkugPk9hx7dneXgNmW6umt3ysYDF+46uggxA6W/vEnrIGa?=
 =?us-ascii?Q?K/QRQfJV00ZGepkmSN3Em5yLXbW/NKdZ6B/DcuQePKew2f6VxXN15QewQ9d5?=
 =?us-ascii?Q?qEGZFryFd0LxXllbbnXHm96TvQBgkSBSJwzOeM8N/BiWwBgcY7ELmIcsakeZ?=
 =?us-ascii?Q?1tHMwoXuMFOAM7inbc3qdG3as2UBYxaSnA1aS4D7HxqH0fe4hTnOVSt9S+5h?=
 =?us-ascii?Q?tWnS+2+fCGXGJYExL24OdTaMq+9OTbtOV0dKx8q9Rlt8gF/dXKM0dBS/nevx?=
 =?us-ascii?Q?hWqvq6hveOorsY/QZ+nGLLgHoO/Pu0qJkwJ8cqbSs4rIUPcXwonaaNMP2WyM?=
 =?us-ascii?Q?4TEJS5fDBpkqXOhNg3UhQFtw9NCcSYxAnWXKgFJWhWj2GYE0GphD9zMRQEjy?=
 =?us-ascii?Q?p2BanIYxJBPYT8dMaoxZHFJdt+fqzfinS27xejtxpgz0jY5z5MKqO5FBGLUi?=
 =?us-ascii?Q?6Z3TUdXWs3iok2eO2vfgwe5s/mMlhVdVE2ceGSfOKdTRGYIeYdKbr1hwBGC1?=
 =?us-ascii?Q?c6ziQySVnh4SesqHm+HwXULDmj0dMul8vomfmXdrRmTmNQbwjWiFZ4ATGUAc?=
 =?us-ascii?Q?AMnMFygqRURWF/nAPJzCAAO1gwCY6K9HaHwry+r7C1slcqZAWNpitl+z8/sy?=
 =?us-ascii?Q?zrCEhz0UNufxbyhLXjN0u0yz8xl7lTBdVKiRWHNosSZXpMWWVpfAQrCflYMg?=
 =?us-ascii?Q?qK5XzuQK6a3QUg8pBwRNB85uJbqC+67YDVgijvWe1f/KmERfhT3Rn7jcJ9AD?=
 =?us-ascii?Q?TyF5usEpdbKhlcz8ThszXehNIKrdFQDOCEcMnZ6/xus/kUx8Xl27s1Um/ERh?=
 =?us-ascii?Q?bSBT+DCzfS6BsoEk+6HwAlMtdyvnHccUFyMS/9pI1IZgu38dxYR6ZyKWMXwA?=
 =?us-ascii?Q?KHRsLV//76VhrXqrhIjJgAqurZBZpq9U0pM0IHidKpDilqym78m+xfa8JZwV?=
 =?us-ascii?Q?IgRKPMxh9LC/uNmKOfYMaHRi8nEVagghPaGujHrx6zDl6GbSsCCimqMTX6Jr?=
 =?us-ascii?Q?6+tkqov7z7HIFUvpERyWAfjOaHm+sdO8o3bKRcEixbl2qoMH5xMzq6ivFGUh?=
 =?us-ascii?Q?SlieFbOIPGaqv/qtU9/z1OulYH6jAEhRYn6inUroTmRhwvuU0TOlRxaV4H0W?=
 =?us-ascii?Q?q/ojLa6sMpDdfOioU2IG0CmtDWRyFbrWyjZH/2CnUohEz7vhAP50zJHAu3sV?=
 =?us-ascii?Q?9qW9KP0+86al9p8nq1M3ZVPFUK0AQ+e676okRLLvGMDh5GUMdDiSsguKvhCQ?=
 =?us-ascii?Q?UDQYiJ9QBnfmBy+Jz1NU6AEhs/UHQV2+wVDkB6UvSIdfNcOUWtLf9Dhh8p3M?=
 =?us-ascii?Q?6AWcmd1XnF2gUAVgfDUXLZWxrx0CftA6pAkpMTWiuuwFMXlIZw3UNtq0KKJR?=
 =?us-ascii?Q?BqXOyXEQ5sDmJx18+dw31as+4X0RxvvYYQ6sQVDauMYIVL1pkTAh70/+W/9T?=
 =?us-ascii?Q?4TJI7zQi6ZMzsBAK8KCF7ll4rpbVnsY=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5aadd0a9-8b00-41d7-84a8-08da26ec3331
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Apr 2022 18:48:34.4366
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: We5eMMh5KfgdoyLH1sWPlgS/szVtZdmfooWPJrwDv9qATaSRHi2R6ZWWbPAz1MvmqadOX77IUyIFRiL3D9FpFA6gm2aqlp7bacr9pwtQO3o=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB4929
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-25_08:2022-04-25,2022-04-25 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxlogscore=881
 suspectscore=0 malwarescore=0 adultscore=0 spamscore=0 mlxscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204250083
X-Proofpoint-ORIG-GUID: XCpykQU_pOeDiTqtbnJVun2tLnQVnexM
X-Proofpoint-GUID: XCpykQU_pOeDiTqtbnJVun2tLnQVnexM
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,
        SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Apr 25, 2022 at 11:25:32PM +0530, Vihas Makwana wrote:
> On Mon, Apr 25, 2022 at 12:30 AM Pavel Skripkin <paskripkin@gmail.com> wrote:
> >
> > Hi Vihas,
> >
> > On 4/24/22 19:31, Vihas Makwana wrote:
> > > There's a NULL check on pmlmepriv in rtw_mlme.c:112 which makes no sense
> > > as rtw_free_mlme_priv_ie_data() dereferences it unconditionally and it
> > > would have already crashed at this point.
> > > Fix this by moving rtw_free_mlme_priv_ie_data() inside the check.
> > >
> > > Signed-off-by: Vihas Makwana <makvihas@gmail.com>
> >
> > That's good catch, but looks like the check is just redundant
> >
> > This function is called only from it's wrapper called
> > rtw_free_mlme_priv() and rtw_free_mlme_priv() is called from 2 places:
> >
> > 4 drivers/staging/r8188eu/os_dep/os_intfs.c|531 col 2|
> > rtw_free_mlme_priv(&padapter->mlmepriv);
> > 5 drivers/staging/r8188eu/os_dep/os_intfs.c|579 col 2|
> > rtw_free_mlme_priv(&padapter->mlmepriv);
> >
> > _Very_ unlikely that `&padapter->mlmepriv` expression will become NULL.
> >
> So I guess either we should remove the check or mark it with the
> `unlikely()` macro.

The likely/unlikely() macros are only for when you can prove it makes a
difference to benchmark.  They hurt readability, but they're important
for optimizing the fast path.

Just remove the check.

regards,
dan carpenter