Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 2/2] i2c: core-smbus: fix a potential missing-check bug
To:     Wolfram Sang <wsa@the-dreams.de>, Wenwen Wang <wang6495@umn.edu>
Cc:     Kangjie Lu <kjlu@umn.edu>,
        "open list:I2C SUBSYSTEM" <linux-i2c@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
References: <1525525341-10046-1-git-send-email-wang6495@umn.edu>
 <20180510111658.sxf3mvye5q6ihxa7@ninjato>
From:   Peter Rosin <peda@axentia.se>
Organization: Axentia Technologies AB
Message-ID: <8f728c2d-b463-507e-2c36-fad22fd99dff@axentia.se>
Date:   Thu, 10 May 2018 15:36:21 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <20180510111658.sxf3mvye5q6ihxa7@ninjato>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: axentia.se does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2018 13:36:27.7428 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: b82b6b8b-f22c-49f7-42f2-08d5b67b08dc
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4ee68585-03e1-4785-942a-df9c1871a234
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0202MB2784
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2018-05-10 13:16, Wolfram Sang wrote:
> On Sat, May 05, 2018 at 08:02:21AM -0500, Wenwen Wang wrote:
>> In i2c_smbus_xfer_emulated(), the function i2c_transfer() is invoked to
>> transfer i2c messages. The number of actual transferred messages is
>> returned and saved to 'status'. If 'status' is negative, that means an
>> error occurred during the transfer process. In that case, the value of
>> 'status' is an error code to indicate the reason of the transfer failure.
>> In most cases, i2c_transfer() can transfer 'num' messages with no error.
>> And so 'status' == 'num'. However, due to unexpected errors, it is probable
>> that only partial messages are transferred by i2c_transfer(). As a result,
>> 'status' != 'num'. This special case is not checked after the invocation of
>> i2c_transfer() and can potentially lead to unexpected issues in the
>> following execution since it is expected that 'status' == 'num'.
>>
>> This patch checks the return value of i2c_transfer() and returns an error
>> code -EIO if the number of actual transferred messages 'status' is not
>> equal to 'num'.
>>
>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> 
> Applied to for-current, thanks!
> 

I meant to comment here but got side-tracked and never got around to it.
But see e.g. [1] and [2] for drivers that will not be happy with this
change. Maybe there are more of those? I did a scan of the drivers in
algos/ and busses/, but there are drivers all over the tree that
implements .master_xfer that I have not audited. Who knows what further
problems this patch will reveal? So, maybe we should be a bit
conservative and only WARN as a first step?

Yes, I know that I suggested this, sorry for not following up with this
in a more timely fashion...

Cheers,
Peter

[1] https://lkml.org/lkml/2018/5/9/871
[2] https://lkml.org/lkml/2018/5/9/877

PS. Also busses/i2c-rcar.c seems like it might return a short "success"
for some sequence of events. But I'm not sure about that one...