Am 09.12.2015 um 00:12 schrieb Paul Bolle:
>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>> @@ -370,19 +370,23 @@ static void gigaset_freecshw(struct cardstate
>> *cs)
>> tasklet_kill(&cs->write_tasklet);
>> if (!cs->hw.ser)
>> return;
>> - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
>> platform_device_unregister(&cs->hw.ser->dev);
>> - kfree(cs->hw.ser);
>> - cs->hw.ser = NULL;
>> }
>>
>> static void gigaset_device_release(struct device *dev)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> + struct cardstate *cs = dev_get_drvdata(dev);
>>
>> /* adapted from platform_device_release() in
>> drivers/base/platform.c */
>> kfree(dev->platform_data);
>> kfree(pdev->resource);
>> +
>> + if (!cs)
>> + return;
>> + dev_set_drvdata(dev, NULL);
>
> dev equals cs->hw.ser->dev.dev, doesn't it?Correct. > So what does setting > cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us? We're freeing cs->hw.ser, not cs->hw.ser->dev. Clearing the reference to cs from the device structure before freeing cs guards against possible use-after-free. >> + kfree(cs->hw.ser); >> + cs->hw.ser = NULL; > > I might be missing something, but what does setting this to NULL buy us > here? Just defensive programming. Guarding against possible use-after-free or double-free. > > (I realize that I'm asking questions to code that isn't actually new but > only moved around, but I think that's still an opportunity to have > another look at that code.) I'm a big fan of one change per patch. If we also want to modify the moved code then that should be done in a separate patch. It makes bisecting so much easier. Same reason why I separated out patch 3/3. And btw same reason why I think patch 1/3 should go in as-is, as an obvious fix to commit f34d7a5b, and any concerns about whether those tests are useful should be addressed by a separate patch. Regards, Tilman -- Tilman Schmidt E-Mail: [email protected] Bonn, Germany Nous, on a des fleurs et des bougies pour nous protéger.
signature.asc
Description: OpenPGP digital signature
