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: <74879c87-689f-6a8e-a177-8bde4c9c4e51@oracle.com>
Date:   Thu, 18 May 2023 11:57:36 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid
To:     Juergen Gross <jgross@suse.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>
Cc:     linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org,
        "James E.J. Bottomley" <jejb@linux.ibm.com>, stable@vger.kernel.org
References: <20230511123432.5793-1-jgross@suse.com>
 <yq1ttwbsoii.fsf@ca-mkp.ca.oracle.com>
 <6614f626-d174-03d0-0993-79e6f6169b71@suse.com>
 <9d356278-c826-dacf-cbe0-79f512b7970e@oracle.com>
 <60aeffe4-b31d-4ea3-d4ea-f50ae25e0316@suse.com>
Content-Language: en-US
From:   John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <60aeffe4-b31d-4ea3-d4ea-f50ae25e0316@suse.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VEJHOHFPRzdjMGRGQ3pLSnBlYUt2a2E1c28zbGRUcC9BbXBYUUJLQTFueW1i?=
 =?utf-8?B?N29UYkVnOVZWWWV0T0g1TFBZV0pWUGNwSzRiZjBXQS9qUWpZWDFneTZOYXRU?=
 =?utf-8?B?b1JkQVBEbFJnenZlWS9HVXZISWZIY0lmRlJzbzgrdUplSXJZZ0puUUJ1cUtF?=
 =?utf-8?B?YS8zWDBtRm50azBqeTlVVEZVbVhRODZ6N2Z0TjRSb0k1NU1pL3U1eXZuaFBm?=
 =?utf-8?B?Z3lrNmFJSlZqNjFQRitxc1JwZXl2Y01BalByZFRzL21nZ2tsUVVCRlhpdUdY?=
 =?utf-8?B?azFLb1R5TFcvQS9UZHQ5ZURHc2RaRWVHdVAweFh1MHhNaHBFTXFqUlpkbzZj?=
 =?utf-8?B?TTUxQk5ieFRpQVp2Q3dnNm9aeStja3pzemZuL2FhMXNLT2JYN0QzM3VKSUgy?=
 =?utf-8?B?R2FwUFdCL1c4cUxhUk5TdE9jaDlYc0JPSjZaa0t2RzZDT0MrMTlGYy9ndTVZ?=
 =?utf-8?B?bmZPZnF3NmJacnNnS3ZkOUtnaWhpZFlwS29QdUNuSUtWYy9Tb0M1VWhqNVpF?=
 =?utf-8?B?SkdGTm9nTjNpbjhkU0x5cGlaTG9DeDBCY0JxMjJOcmxwL21MT3ZaVkJ4OEpv?=
 =?utf-8?B?c1N0VlllOURhUWkzVFl0SG9LV0UwWkNENm9LTTVVUEwxbnNnVWgrbXZwdFBm?=
 =?utf-8?B?ZzA2Q0JURk1pYTRnUSswZnNpY2lkZlcyOGdaSHlROTF1RjFzaDZFMWJTN1hy?=
 =?utf-8?B?b0dMZXFRc3N2SEcwSVdxbWNWakFnMlFoMjg4ejViQjBqRyt2bTVlZGFDNGFW?=
 =?utf-8?B?NkdqYlVqTmp3WGZpRXJPdldXMGNkMk8rTks2NzZoSE1YYXpTK0NnVU5UclZJ?=
 =?utf-8?B?WStocG9KQnd2eVN4ZURnSUE3bWN1SUNHSUZYdUlEODVUVi9rZTZNOVd1UkJF?=
 =?utf-8?B?YmlENTM5WDBuSFZvUmxlV0ZGREF6MmppTkJPbmFlcWdUV0JHSDY2Z3BOcHZJ?=
 =?utf-8?B?c2RQY3NJV0N4RE82VVY1TjJFUGtOVUltNWtVWUxaMkNsWVlMY0tYOVhyTS80?=
 =?utf-8?B?SGNEUisvbjNJQVRVaFBpMDFFdnlIYXUvWXBiditra2NyOWttcVF0am5uamtU?=
 =?utf-8?B?enNsZ0dhQXFZNXNyUjIxdUxEN1NKd1p0V0hTTEMwd2prMkluRlhickd5M2F1?=
 =?utf-8?B?cDlHdk80ZnRIdm1INXJCaEM0OERLSDdKTjhXVDlLbTZENU11b29hNzRKaG45?=
 =?utf-8?B?azQyRkRJSi9GSDBqMGdsWU1Uc2V5S2g0TnJNL2FvOElDZE1PNzNlQUwweGQv?=
 =?utf-8?B?TitxZGdwZ2ZIMFBxcFBKY2tCNk9zeFJCT0o5b0VUWi94cUJEb0NSOVQ2NzJT?=
 =?utf-8?B?b3lKMXlQV3pBLzBJZC92TEpCcVIxOHYwbzdrOW5pK1h5NUlTTTV6RWxEY3ZG?=
 =?utf-8?B?VDk3Z2hjMjZkKzExdE5HWEl5SFV6R0lQVTIyZ1hmTkdIV0NDcnBPVzMxUkNF?=
 =?utf-8?B?aDRwZVlkcjJ6OXF2WTAxOUxZeWtHZkFTQ0EwQW9IVVFTODdCM2NSYWUxVUhm?=
 =?utf-8?B?SXoyUGxuMWVFUEJhMzdmOEUybHRFK2ROTDFocDJEdWVEVEZkUVFEY2l0R1Ax?=
 =?utf-8?B?LzJua0VSek1Rd2JLbmdpU3FjSHlXVnZpWTVYMkJtT0xzL1FuZVJPQUN2U1Jl?=
 =?utf-8?B?aGZVaWhzSGNSRUYycVdRTk1HSnVzSVAyK3Nhc2NPbzU0UTIxZk02MURuTXdi?=
 =?utf-8?B?S3lvSllOeE5mT3F0aXFKQ2lhdmlmVENmZjZCcVZabHk0M1NjTnVhSDFQWWda?=
 =?utf-8?B?ZlhndkhSQk5SU29kaURKb2xKS0cwTmxRWXV4N2hsdWw3NDRHQTJGVC9HSEZr?=
 =?utf-8?B?NUI5QmtDVTVSQmFZeVE3aXpMejVCV0NLcW8wWUc0MzQzTWw2cWk2eGp6VEZF?=
 =?utf-8?B?M2xVekc2NGROTEwxekdWWHQvc3UwVU1oeVVjSlZJdHZ2QVd5VDI5a1dod29S?=
 =?utf-8?B?MU9xSEkySUMxVEI3T1hwRjBaOXZHWU80Q1E2d1p1cVRxQ3AvaGlRN2NJWlJV?=
 =?utf-8?B?TmY2ZXRibkVGWUNGM0ZtaEpHS1ozMzNzaGcxYk02ZU4zeFd1U3VjdkhXRDlt?=
 =?utf-8?B?ajV5bi9DTk5yb0ZnbHY3VGIvUTQvaEIyOEhNT3hLV2FTeEVoN1kzRDBvWG8r?=
 =?utf-8?Q?T71TIJQ564ENerhU2NLr9baXr?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: htWoGPeGRc7AKw3RUbx3IdY7Fo1icGfHjpLnbCUrHc2fbDjNjMODzmifUOTU7Qw80XQGXytl6v28D5WAnoKFV2i7gZHLtTVzrkyd2h2386bNqZh+1ESn9LHC4Ym2147WtaZPfYqhS7KQXZjjaHGv24b5tnjKaszPkmAxINosAbFHUzMamPtwtqowSQG5CVT1liV1cx+D34kuusWoIkjLnaG2LenrtJIJOoRFOhciYZFqvxTXYwJ9DM4ate4elxOm0qVo/d+A51a9K0nKJ9vHYt2eRKwliGu4vmLuZkWwlrK5vzamudDzHOLmEl4xtl+Vt7oPDs9BiXb85no4fyMr1pKIyDng07mUWFo+bXb8YAIv7xAmMLetTGcmoIqUeTK1hXr0p0ffqYJasrDUn/jKTJ6+/ZgZjXHJgDixhVYDBc5uryvZ8kmKR2+q+YdCF6xwxWkM5gVUvkUTreQMe5rVIXIaTSWLZGcBE6hSZlgpoBbyZougN9MTH2wn4KkhK3PuHI5VNwh6XBFZxHgXaTUwiu5lEmSkI4S5vBFF2qE85hnfRC6e1jn+bYEdnTpyOkCP5mDOczk2wVvsAgEGIIBB9S7a7Uv+x2LKLinH2sE2LTx8pREh9S9pQdwGPWNXBFU7A8Y5CVU69Vvtt5e17VVNggmqke9QgOFIwwRBzleNVvsAWuF+1wmrL+6kq6b95KuEEX44HfIYSWrRgeBjdn4qnIfxHKe7oCni5+ipO317pxhxXM+48vypGkv6ixi5a9tzXSNp1z4LI+OO9NKoiMa7zSTcWwV84gwkzcwdou7tEUsFw8BFA7nM1GZzmuUh8Rm/uMcP0kImY0hPGwp4QQfyEQ==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 66d68962-7cf3-4f45-8ca5-08db578eb3a3
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 May 2023 10:57:41.6399
 (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: oT3H21otOIDGbWCbTtMd5w4712MO7DbalQiVUf2OexFtzsIhqRJ1kSnXSvABg0Y7Fp6W48joJAUZeAvb3JzAsw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB4943
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-18_08,2023-05-17_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 spamscore=0
 bulkscore=0 suspectscore=0 mlxlogscore=863 adultscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2304280000
 definitions=main-2305180084
X-Proofpoint-ORIG-GUID: 1f9B7HId8j9ntUvQFPBz1wLXOhXa_ulF
X-Proofpoint-GUID: 1f9B7HId8j9ntUvQFPBz1wLXOhXa_ulF
X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE 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 18/05/2023 05:53, Juergen Gross wrote:
>>
>> Is there a reason that callers of scsi_execute_cmd() are not always 
>> checking the result for a negative error code (before examining the 
>> buffer)?
> 
> I don't know.
> 
> I've stumbled over the problem while looking into the code due to 
> analyzing a
> customer's problem. I'm no SCSI expert, but the customer was running Xen 
> and
> there was the suspicion this could be an underlying Xen issue (which is my
> area of interest).
> 
> It became clear rather quickly that the uninitialized sshdr wasn't the root
> cause of the customer's problems, but I thought it should be fixed 
> anyway. As
> there seem to be quite some problematic callers of scsi_execute_cmd(), I've
> chosen to add the minimal needed initialization of sshdr to 
> scsi_execute_cmd()
> instead of trying to fix all callers.

ok, understood. I am looking through this thread again, and you seem to 
have to repeat yourself - sorry about that.

So I don't think that this code has changed from commit 3949e2, as you say.

I think it's better to fix up the callers. Further to that, I dislike 
how we pass a pointer to this local sshdr structure. I would prefer if 
scsi_execute_cmd() could kmalloc() the mem for these buffers and the 
callers could handle free'ing them - I can put together a patch for 
that, to see what people think.

@Martin, Do you have any preference for what we do now? This code which 
does not check for error and does not pre-zero sshdr is longstanding, so 
I am not sure if Juergen's change is required for for v6.4. I'm thinking 
to fix callers for v6.5 and also maybe change the API, as I described.

Thanks,
John