Subject: Re: [PATCH v3 2/5] Docs: usb: update comment and code near decrement
 our usage count for the device
To:     Philipp Hortmann <philipp.g.hortmann@gmail.com>, corbet@lwn.net,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     linux-usb@vger.kernel.org, gregkh@linuxfoundation.org
References: <cover.1638771720.git.philipp.g.hortmann@gmail.com>
 <ca8fd26ccff6521c7477a2035e703e099da56214.1638771720.git.philipp.g.hortmann@gmail.com>
From:   Oliver Neukum <oneukum@suse.com>
Message-ID: <bc30a2f4-a913-1f5e-c1fa-e10f8f357128@suse.com>
Date:   Tue, 7 Dec 2021 10:30:26 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
In-Reply-To: <ca8fd26ccff6521c7477a2035e703e099da56214.1638771720.git.philipp.g.hortmann@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MlZuamtuQXBoaTNSYjZ4Q2ZqUmtDSldsM1FmWFpEZWNKa2FBNkRDcUgzMUVD?=
 =?utf-8?B?c0VhZ0xzSFNCeENCMEh2cnZ1eTVnNjZ4SVVFSlVlK0VIVTFRUmorUnAwbWxF?=
 =?utf-8?B?eWd0NmUwUFJzK0JLTURtVm5JekhDVzI3aThNNkw0ZStvNTFEL2VaRE9QaDFj?=
 =?utf-8?B?YmxwakZobEVwZEZNY1oyUGowZHJxckljNmVWRlpZNGdid2RhZmMwZnQwZ2tJ?=
 =?utf-8?B?RHFDV09JdVhQRDhJbUdMa1c5ZXFwcGNBazZiUitJcnM4REVRZHQ1ZE9jR1da?=
 =?utf-8?B?ZGVzU2hsTWM3Y1JHbUNzclBkaWs1Nytkd2pLOUVhVEFxVjlBTmU5eFlUUlQ3?=
 =?utf-8?B?ekNYU3BJTlZvK05oSDFzbG9zNGh6OEhVWXF2cVloSk1ObDhoY2pNZGE3NzJ1?=
 =?utf-8?B?Q0s2M3dpdmRsaW9iNGE4YjI2UkY3R0VUZjdHUkE4RE5JUVVIeFI3UjB5OGhV?=
 =?utf-8?B?UUl6a2JreWl6NkIvSVlWKzhHSzFxYWlUL3RuRWYrMlR5eGM5TzQ3YmZrVEJh?=
 =?utf-8?B?akJudEozNy9qWktrREgrR3hmZTd0NHU0RU1GZW1WQXJaQTRXOGtsR0Ewbk04?=
 =?utf-8?B?aUtlNXVOakl0K0R0YTQxcXluS0tEYitJOFNJZG5WNW9NRUp6V0MxQ1pWRktW?=
 =?utf-8?B?SWRsT0thdnZRajVQeHpYTTNubFZqanNaTnJ6S3ZlN0V6RkM1UTQ4MEJOdm5F?=
 =?utf-8?B?UGZaQ0dtVU1hMjFDOE9yRFc4Tk54ak5CSmY4cGVrTHV6UzYxWElvSDBGbmVP?=
 =?utf-8?B?aGpRaVhRR3NzMko0d05sU053SWRRNEpld25lNExscHVGeHBFQWJCc05BSFpw?=
 =?utf-8?B?Zy9tU09ISlpZbmU0aWhidllzak9BVm9hZkJKT0xWUHlFSzVqMnpNQ1NaVExW?=
 =?utf-8?B?RUoyZkxVMzRtNnFFdDhHa1RhWmk1WGJmS2ZubjF4a1ZnOHJrL2JpbGRWd3Va?=
 =?utf-8?B?MU1MYjZDcGVpQStIdnJlRzh3OFJINDROakQ5S0NTUENWVVFpVjdEQzJ3Tzdv?=
 =?utf-8?B?dlZWVlBXYWlrdDdEZkhmZXFWVUJKQ3RRc3ZSdVdveGliYVNVSmo5WXNTd0xC?=
 =?utf-8?B?eHptUDBlaFlvdXFUR2w0elFONHJMZ284TmJ3R2swdXRJZnVQQWFGMGZSTmtj?=
 =?utf-8?B?TkIvTHEvWXZlcWM1UGZQWXE2aE5manlkK3RDR0t2b1J5d1E5WXBEbjlWbUEv?=
 =?utf-8?B?S2pBUVN6Q203OU1wN3plb2FxZWk3OElDN1YzTW9WbzBwZjhDZG0wV3NRMW9y?=
 =?utf-8?B?V3FFWWVMYmlEZFB6YWxROTRtU0lhd2V4azBZeHpCMEFBOEFzci8xeU50dmND?=
 =?utf-8?B?c29lWG84RDRLRkpsYXFJdTNnallxUk5rVm5oQSs0TTNSeDRHOTZrYUNuNit4?=
 =?utf-8?B?eTFTN0JGYlE2R3N1M2IzaFVkYjJMYmljeFVyOTBHbjZsaTd1d1hJRU1zQkJY?=
 =?utf-8?B?MmVpcDIwdUdCK3E4bmhpNlAyb1BzZkJtK1ZIZlNUMU5ibUIzTC9QRzh1eUMy?=
 =?utf-8?B?Y1doVTAzS0dST1lQVFE3OGVWYlFPVjJBVEJ1cmErZGYrbU8ra3dTNGFFdDhx?=
 =?utf-8?B?ZFBUU09OTnY1bGwxQWgzWnpjNTdyRVhDODZCNmQ1ajVxZjZIRTliS3NZOXRs?=
 =?utf-8?B?UGZiMVRDWkhxRnJyNWhyc1lScGt1NWJNVUl2ZjloWVo3bVVFVVIzZ3ZKampK?=
 =?utf-8?B?WjhyZEJva2xFNklaWjhXUWpzaUNkZ3ZHd1Fvajl4MVFyeEpEeWZoUFBnZ2Vs?=
 =?utf-8?B?MTlIYXFzUGs1V0dlUmVRNFFNa3NYOGZNaVpzYUlyN01Vc0xJV1g2REtRdVFt?=
 =?utf-8?B?TkthTkZNNWp6LytpZG5vdFhmOEwyd3hTRm1PRjhlSFpMb0VqYXdkY3J2ZnBQ?=
 =?utf-8?B?MG9lMFNxalZoNVRTcVVva3Z0R0F6cnhuMWdVY3BRcUJpeUozYUJlb1R1NjVl?=
 =?utf-8?B?TTk0ZXRNRUtBSmpaNWNrNVhOd205YXhqbWF1L09tbHY2M0JPMUVTQTdZUjRo?=
 =?utf-8?B?aFAzUnhZZ3ppdThGTWpaOFhpNVB3dWMvTmNvV3dDUkdMemtvLzN0NEpGa0ZZ?=
 =?utf-8?B?eXZHWS91Ny9aTG5wVjhvcVFrNkZIR3pYUWlUSkF5c2JvSjhiTmhjVHQ1SnNX?=
 =?utf-8?B?Y2w2U0QyN3R4VWlDNG1qcUhCaG5OMHFXK01WcFc2OHloUkw0WHlrS1JqQUFX?=
 =?utf-8?B?bTBrd2g1ZC90MjRpdlJSK1luSDJIRENOdjBuQmRKa0RDZEhCdTNRQVQ3TkQ5?=
 =?utf-8?Q?u5gUBi5yzPiedGB828GYxk97j/Wlsip1h+dwG6rwJI=3D?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 645760de-cecd-4de6-eef1-08d9b9643558
X-MS-Exchange-CrossTenant-AuthSource: DB7PR04MB5050.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Dec 2021 09:30:29.4033
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qDEQMLLNl9+CgFWkVSGqek4QRC6fkhh4o/2MsPSpF7acuz3fLfPrC4hIQC231vqIqJkmrxQ1qDlVSsGMl/MW0w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3PR0402MB3738
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 06.12.21 21:57, Philipp Hortmann wrote:

> Update comment: decrement our usage count ..
> and code according to usb-skeleton.c

Hi,

and that is exactly the problem, I am afraid.
Your patch would be correct if the underlying code were correct.

>  
> -    /* decrement our usage count for the device */
> -    --skel->open_count;
> +    /* decrement the count on our device */
> +    kref_put(&dev->kref, skel_delete);
>  
>  
>  One of the more difficult problems that USB drivers must be able to

I am sorry but the code in usb-skel.c is wrong. You grab a reference
in skel_open():

        /* increment our usage count for the device */
        kref_get(&dev->kref);

which is good, but in skel_release() we do:

        /* decrement the count on our device */
        kref_put(&dev->kref, skel_delete);

unconditionally.

Think this through:

- Device is plugged in -> device node and internal data is created
- open() called -> kref_get(), we get a reference
- close() -> kref_put() -> refcount goes to zero -> skel_delete() is called, struct usb_skel is freed:

static void skel_delete(struct kref *kref)
{
        struct usb_skel *dev = to_skel_dev(kref);

        usb_free_urb(dev->bulk_in_urb);
        usb_put_intf(dev->interface);
        usb_put_dev(dev->udev);
        kfree(dev->bulk_in_buffer);
        kfree(dev);
}

with intfdata left intact.

- open() is called again -> We are following a dangling pointer into cloud cuckoo land.

Unfortunately this code is older than git, so I cannot just send a revert.
What to do?

	Regards
		Oliver