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;
Message-ID: <8d8f8c11-6ca5-5770-989c-704f76436cc4@suse.com>
Date:   Wed, 30 Nov 2022 10:34:41 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH] hvc/xen: prevent concurrent accesses to the shared ring
Content-Language: en-US
To:     =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= <roger.pau@citrix.com>
Cc:     linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        Juergen Gross <jgross@suse.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Olof Johansson <olof@lixom.net>, Ingo Molnar <mingo@elte.hu>,
        Chris Wright <chrisw@sous-sol.org>,
        linuxppc-dev@lists.ozlabs.org,
        Stefano Stabellini <sstabellini@kernel.org>
References: <20221129120107.2942-1-roger.pau@citrix.com>
 <alpine.DEB.2.22.394.2211291351160.4039@ubuntu-linux-20-04-desktop>
 <Y4chryL2YvfhgQmk@Air-de-Roger>
From:   Jan Beulich <jbeulich@suse.com>
In-Reply-To: <Y4chryL2YvfhgQmk@Air-de-Roger>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UEtqby9vZDZSK3hCQ3RSendydkZLcGdPUDVOZk1lbUNVc09Na1hMOXB3TjJS?=
 =?utf-8?B?YmYyL2pMN1ZtYkFsdkwxVG1ERDFFa1pLVFQrWERrMmVFQ3QrMFBYaEpCcFM4?=
 =?utf-8?B?cjlXNVRudmg4YW9YTXk1WDRpdmgwZ3pCSUZBQ2g0NkZkcnBPWFVmbStaMWRt?=
 =?utf-8?B?U05mK2RHc216V0ZjZlJsaGkvL2NXSjRzVHE4NG9vZURwemtITGRUa2t4UmdX?=
 =?utf-8?B?OGQzRXJLVHRKWk9zRmNCdkZ0bkw0M0E0S3JSRnkwSk54OHo3bEs5Z3lBZnlz?=
 =?utf-8?B?ZjByNkEwbmN0WWRhT2RBUUNGejZDaWdVU3IwQUczZUhsZUtIdFRocWdiUUN1?=
 =?utf-8?B?dGk2eUI3MWx6c0FmandVblY1Q0ZieXA0S25JejdGamRVS2czVk9Ud1NFaEJi?=
 =?utf-8?B?YVl5c1U2TCtOakVEZW4yNVM3bmdpQVlpV1Z6QnBNRUFsRmFyOEJnOUZVYTQ1?=
 =?utf-8?B?T0FLRngydzlKUlJkVGlQcFpnV2JMQURJVUhBWnNOWWM1ekNEaU1vckFwTkVF?=
 =?utf-8?B?SmU3SHdEaU1vL1NWUG83Ykc3WHYxYWtXVDJLU0c5SkJGdWFKL3FZcmZGNG01?=
 =?utf-8?B?UkZzaWRqR1dERW5yZWhDUk10M3hFVk44T2tEWENtZmRQa29qZ1l1alI4cUlw?=
 =?utf-8?B?aWxaMFNCYUNDam1WaVh5bFhJK1JwYzZMQ1ZRcHptalQ5cmRLeEEvYXVxbTJ4?=
 =?utf-8?B?ZGVRRnNvemE4SEFXZ1ZMUzRvakxiTWtMWE1GY29yK1VJKzQ5RklNUytKdWRF?=
 =?utf-8?B?R2RVMHY4WWc4R3plQ1NnYXBOT0YyWW9KN2lUT2FESXRJOWE5cWxOTlNpUGts?=
 =?utf-8?B?TW9TQythcDVsZGE3b05qU0pxY0k0YzRoNCt6VkRTZTYrSUE2NmhYSEV3SW1W?=
 =?utf-8?B?cGt3K2F3L3N0UUlzbE82R2Z4ZzBiWXdueHZyMllITTkyeW91SkFnbTFETE1U?=
 =?utf-8?B?b0ZjL2NBY1diV1RMTi9xUDYzdCtHVmVmNWlRRmppZDlJbERtbzdrMXFPNzVR?=
 =?utf-8?B?eUFGWHZMVi8rRUF5S0xYbzZ0djEzUjhTbVhvZ1IwRzAySGpBeXhiMVgyd0hq?=
 =?utf-8?B?bjhqZVEvR2lYMGE1aXV6akVNNW9CWGN1WUViTVFrRGtpOE9odlZ5MWtwTko2?=
 =?utf-8?B?dktqWVk5QURPQkkvdlJoMGpmU1lZSy9kdzgwMHpBZTdyQ1ovbFpYNm1zMWpo?=
 =?utf-8?B?cDZOZzZvdmFaWlBGVHUxbHRIRGNZUktVdmh3b1lCeFBIMnFTZ1BtY3pXaWd6?=
 =?utf-8?B?QTFZb2xZM054NHpmYTdpanNCZXNVakhSYmllaUhTQ1V2STkvcTFvYjN5dnNX?=
 =?utf-8?B?SjZhQmRSR29Tb2VRVXM5UVA4VFIxeU9SYXkrWEVpell3em9DeUpSVDd1QURL?=
 =?utf-8?B?dnJxZDBuVWwvcWYxaGxZMm1WOWplM1Jjd2NnQW0zdlp1cE5CcnhIVTV3RG85?=
 =?utf-8?B?M25VQVRzWnNqeGQvNnhpVmNwanBXejF2SThXUGJsaTU1QUVFMy9xdmt1UHFz?=
 =?utf-8?B?WlpwT1Z1RVdPd0kyNURCaGVHR1EyVy8yUHVWanZvQ1pwdFNWUlJCK2NtVlVr?=
 =?utf-8?B?U3g5M1dyczdrWFliTi9NUlFRRnUrVmpZUEdsTldQaXVTcmg1azM5WlBYd0J3?=
 =?utf-8?B?VTZCem93My9xMGthMTBjeTJtN1FBanNtVUZrN1ArMzFaanRaeGp0ejJhL2NV?=
 =?utf-8?B?VDc5T0wwMVd6aGxBN2NyanlkbFdNcmtKTFBuQTFIaER4dUFodTROQ0pQUGEy?=
 =?utf-8?B?SzlId1IyTjVMZ21sdmhxblcreU1Xbjd5MUtJNU02UWtObURUaU85d3JLNjVE?=
 =?utf-8?B?S3pHeGZFUENPeTN5Wm1MSDBIRDJWYVVNWjl2LzV0WFQ1T0NibU4wV01zYTdF?=
 =?utf-8?B?dVFIMkprU3FJa2VEeHptUWl6bTYrN3Y1ZU1TbmpCb1haS3g3RDJDTXBLbFZw?=
 =?utf-8?B?bmVLK2FyckxxbmZqUnNXTzNxVFh3NFprcHpZZHJBbk9vdklGK3F5N0I5ZDVG?=
 =?utf-8?B?OC9XeEFrOEZWamV3dHdKeFp3bGF2SVYxSGlWaEhYYmZHY0JTYitFTE5EZmRp?=
 =?utf-8?B?ZXRnVjdGdWc1KzRSanBBK1E1QlJwY3hFN0ZPMnRwdTJta3dRZHNzQTcvRmtF?=
 =?utf-8?Q?z/IdkW6c5+RN5ZytWV4IF+HjQ?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 74c3cc18-2753-4f49-67d2-08dad2b61c9c
X-MS-Exchange-CrossTenant-AuthSource: VE1PR04MB6560.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Nov 2022 09:34:43.3801
 (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: 4LVrTgK2wnni4jBSNaCsK7u25Wt84dlMzymOvuL1FRnZyBfDknM05VZqulWqpqZu/MSgmweHQ7HePkM0XxZ37A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB8535
X-Spam-Status: No, score=-2.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS 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 30.11.2022 10:26, Roger Pau Monné wrote:
> On Tue, Nov 29, 2022 at 02:12:10PM -0800, Stefano Stabellini wrote:
>> On Tue, 29 Nov 2022, Roger Pau Monne wrote:
>>> The hvc machinery registers both a console and a tty device based on
>>> the hv ops provided by the specific implementation.  Those two
>>> interfaces however have different locks, and there's no single locks
>>> that's shared between the tty and the console implementations, hence
>>> the driver needs to protect itself against concurrent accesses.
>>> Otherwise concurrent calls using the split interfaces are likely to
>>> corrupt the ring indexes, leaving the console unusable.
>>>
>>> Introduce a lock to xencons_info to serialize accesses to the shared
>>> ring.  This is only required when using the shared memory console,
>>> concurrent accesses to the hypercall based console implementation are
>>> not an issue.
>>>
>>> Note the conditional logic in domU_read_console() is slightly modified
>>> so the notify_daemon() call can be done outside of the locked region:
>>> it's an hypercall and there's no need for it to be done with the lock
>>> held.
>>>
>>> Fixes: b536b4b96230 ('xen: use the hvc console infrastructure for Xen console')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> While the write handler (domU_write_console()) is used by both the
>>> console and the tty ops, that's not the case for the read side
>>> (domU_read_console()).  It's not obvious to me whether we could get
>>> concurrent poll calls from the poll_get_char tty hook, hence stay on
>>> the safe side also serialize read accesses in domU_read_console().
>>
>> I think domU_read_console doesn't need it. struct hv_ops and struct
>> console are both already locked although independently locked.
>>
>> I think we shouldn't add an unrequired lock there.
> 
> Not all accesses are done using the tty lock.  There's a path using
> tty_find_polling_driver() in kgdboc.c that directly calls into the
> ->poll_get_char() hook without any locks apparently taken.

Simply by the name of the file I'm inclined to say that debugger code
not respecting locks may be kind of intentional (but would then need
to be accompanied by certain other precautions there).

Jan