Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 26 Aug 2021 14:21:27 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     David Laight <David.Laight@ACULAB.COM>
Cc:     'Pavel Skripkin' <paskripkin@gmail.com>,
        "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>,
        "phil@philpotter.co.uk" <phil@philpotter.co.uk>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "straube.linux@gmail.com" <straube.linux@gmail.com>,
        "fmdefrancesco@gmail.com" <fmdefrancesco@gmail.com>,
        "linux-staging@lists.linux.dev" <linux-staging@lists.linux.dev>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 3/6] staging: r8188eu: add error handling of rtw_read8
Message-ID: <20210826112127.GZ1931@kadam>
References: <cover.1629789580.git.paskripkin@gmail.com>
 <c59d5d850bf9aab208704182c83086609289cb9c.1629789580.git.paskripkin@gmail.com>
 <f5c20e0f1b934e04ad11a3627d71f5db@AcuMS.aculab.com>
 <20210826112745.5e1421ed@gmail.com>
 <a251767ca181405b93b5ea55a26c3ff0@AcuMS.aculab.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a251767ca181405b93b5ea55a26c3ff0@AcuMS.aculab.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from kadam (62.8.83.99) by JNAP275CA0002.ZAFP275.PROD.OUTLOOK.COM (2603:1086:0:4c::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4457.20 via Frontend Transport; Thu, 26 Aug 2021 11:21:39 +0000
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 6946d79b-3002-437d-8607-08d96883af8c
X-MS-TrafficTypeDiagnostic: MWHPR10MB1550:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <MWHPR10MB15501A439C5B483C9474F66C8EC79@MWHPR10MB1550.namprd10.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:7691;
X-MS-Exchange-SenderADCheck: 1
X-MS-Exchange-AntiSpam-Relay: 0
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: v3y4GQaC+ftCz/BTRExkmZWW4epwj6WzeXwc6DSWdOyHX7A4ozvqYYUxLQXb446LVC//hv7fUE99mWd3/gvztT7dK+QseMmLwnzG55Nkft+FYJoEUh3/K2EMGHeMRunmlJ5utKIO8DRxDIMU1t6rCPvf4B9B2i3hM81WnPZJQDVXYFBAnAyNbKKGWFSOVVZOAy/O9Q8P5TfaiPppqg+fe+w0hUpMddxIu1P4UZluypHaO0kf7CVZ/WS+SpebT2YMJZ7WACiPF0RIEiOgYCBTSH5E5sTJ5DuXe7lMaOgk56PXen5hv5Rry1Qt34PXP09LKxqf1BdPtbJQ6hW6P9bRh4gm9RtZSsO3QcFCqlnUO4jVFcAolk7dBxkrWhtjkHFPaQ1ieRWLAg3VKeICA+DLwtEr59KC99VlusH2c55xuDTLcE6tSTPp/q9pY/qSdsvjuzpEke1IbBTK7EfJBOwvzX665lex+5bq/grU9g/08rg2yif0u5g9M0X/eOCpQjexx1niRlutOIEJI7nmlJNiW2fa7+1xI//t6zUdQ0s4eh26BU/khRuUyZQAXOVQSPxnt28zEoJ/CIA80Rp91/dE1Z/UVFlut5KdWNZXbydj08e1el6dis7wEsMcVVTc70t89+/VRe1ByEIXWxmTcFKKQPSRj/5Q4LehPkc62eu90jAYsbfRrRIpzV9WyM3e3lQjZjbMZOvwpUmEB2QAyzkpnw==
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MWHPR1001MB2365.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(39860400002)(396003)(346002)(136003)(366004)(376002)(38350700002)(86362001)(83380400001)(956004)(8936002)(8676002)(38100700002)(1076003)(186003)(6666004)(33716001)(66476007)(6496006)(55016002)(52116002)(9686003)(2906002)(6916009)(54906003)(9576002)(316002)(5660300002)(4326008)(44832011)(66946007)(66556008)(26005)(478600001)(33656002);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Qcx4c9+5xK7a/0Ms2NAuA0yl7SqdREbp5y4GOK3r0+opZViUAI8oiQdPgA2y?=
 =?us-ascii?Q?35Bl4x5Yelt4Q9cv18JQAvyPWDJLZxyCTQAVBHIEd8A6dE9RpyKgvm6XDIs1?=
 =?us-ascii?Q?FxVtCBhQ2dygnv9I/JNxjDFq/VtDBQQIxjp6ZBJghSAI3AeDKMBWMxuEbKej?=
 =?us-ascii?Q?MKcmsHe911X1mZZY7yyjJ1o4JB54fppFYyqq+RVnhUS189WSiMhjtNBHiXSR?=
 =?us-ascii?Q?lpExNyVKRO4rD7qtwHcJeffCOPzllHYjNWtpHHOJ2tueL589Kt46u7ZGiBhx?=
 =?us-ascii?Q?WOGyYG2yrev9Xbz2AxxNUb1yuiakOsNrJ4wjAQVld7hJf+2UksIaox31qxb2?=
 =?us-ascii?Q?GflCDhlrO54ILZBCtRUexwmYg1aI+IcmgiIrEePWKsviHHc/9tBI9x/KtPAv?=
 =?us-ascii?Q?pfM3BxzJdVp8rZMxARQCtmZroIxA2wiJlzWWvnA/HVkBgm7PBhUI5beQ/lRB?=
 =?us-ascii?Q?IGfRmXwnNICBTHcQw2FD5JoM1ySip9bQhKgQt39Cgyn3Y6LS9JncMzXewawn?=
 =?us-ascii?Q?21DVmdDA59NsVBBao1fvVfmBP2qQHP0nZ0uY8UlZs9vah03IJOZDAPgpISgr?=
 =?us-ascii?Q?Xgk1qMv+jxFe/4UvJ3Gas13dP+pdpuJIW2kexu/dUEr4lGAo5Wsr640p2dat?=
 =?us-ascii?Q?6Grd7liIuOlcgM1wGWjM17tpoUzaeGp1VPzSJLVtqphLw3r27X9duzveHaMP?=
 =?us-ascii?Q?fnuom1cwwR/4cAaGjpcvE2xhBIaeXISzGEAvEB+f7ciRUZ69gzcx54gBgHU2?=
 =?us-ascii?Q?t4pS5D/9eiaLkPu5icoHPhiIVG70TeJ7xfzeN5W8ovHaMgNFCCny7LteUtJh?=
 =?us-ascii?Q?HSJIh/Y38oPH1OjcqvwKx0RxtwDuJDwP04aUshoO41tLUgBEvLFWZ5kWl5Ks?=
 =?us-ascii?Q?PllTN2hv2Z5CR+xMePD7VApClA45uPE9OC0QNbwjE2KE+JQ1Rsf7uLLOfhmv?=
 =?us-ascii?Q?mhYXqXsWwghaNZ20sS4PNIsFH997e8ngxKFYhmvKwKMA9+Wt6abiYdveVd1g?=
 =?us-ascii?Q?9f3OhIRSqKQCZT4kG8o8d3KnhgIBtueeqwVgPPmHndmHKCV1idyfBSXVwEE8?=
 =?us-ascii?Q?ciDHuGF2IVhTW4qGuCpOvAFeHkYZFVVodqZ5jsBNqK3eXkw1W1IPUgY75IKc?=
 =?us-ascii?Q?zmE2NjJgIWz0k1cUsKsT5us1BGJ/8AAGlcTvlkujcMWrjP5bMWF30r8C4/8S?=
 =?us-ascii?Q?LPIZ7yiVgLJ1W6p9CfnL8e2JCcReCuW3o/woGKAR6ZJiBXSrTD4J6wjO0cPg?=
 =?us-ascii?Q?mZ4X2CFsro0AToXVwRCW80RsWcZqWZNxcM/zB3rpm3O8Cr7stIF+XfD7LxHv?=
 =?us-ascii?Q?6vt7qfBBB6PmIqGBoEX5oX6f?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6946d79b-3002-437d-8607-08d96883af8c
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Aug 2021 11:21:44.8251
 (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: vQceaAY9G/CVzZzHW9u+8YY8+chriFXYnyT589/+lYMqwNR2f70IW6I+ARK2rTTEInUvnPX5vLRW+mPKhVWFu51QMzy3l2HqHUp8qqD0mh8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1550
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10087 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 spamscore=0
 mlxlogscore=999 bulkscore=0 mlxscore=0 adultscore=0 malwarescore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2107140000 definitions=main-2108260069
X-Proofpoint-GUID: vTjgK3wpiJK6BUQnKiqq2nt6Wg5dcPvs
X-Proofpoint-ORIG-GUID: vTjgK3wpiJK6BUQnKiqq2nt6Wg5dcPvs
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Aug 26, 2021 at 10:19:40AM +0000, David Laight wrote:
> From: Pavel Skripkin
> > Sent: 26 August 2021 09:28
> > 
> > On Thu, 26 Aug 2021 08:21:34 +0000
> > David Laight <David.Laight@ACULAB.COM> wrote:
> > 
> > > From: Pavel Skripkin
> > > > Sent: 24 August 2021 08:27
> > > >
> > > > _rtw_read8 function can fail in case of usb transfer failure. But
> > > > previous function prototype wasn't designed to return an error to
> > > > caller. It can cause a lot uninit value bugs all across the driver
> > > > code, since rtw_read8() returns local stack variable to caller.
> > > >
> > > > Fix it by changing the prototype of this function. Now it returns an
> > > > int: 0 on success, negative error value on failure and callers
> > > > should pass the pointer to storage location for register value.
> > >
> > > ...
> > > > +		len += snprintf(page + len, count - len,
> > > > "rtw_read8(0x%x)=0x%x\n",
> > > > +				proc_get_read_addr, (u8) tmp);
> > >
> > > That is broken.
> > >
> > 
> > Don't get it, sorry. Previous code did exactly the same thing, but
> > didn't check if read() was successful.
> 
> Look up the return value of snprintf().
> 

It's hard to understand what you are saying.  I think you are confusing
libc snprintf with the kernel snprintf?  In libc the snprintf function
can return negatives but in the kernel it cannot.  This is not going
to change.  Any code which checks for negative snprintf returns in the
kernel is wrong and should be fixed.

Anyway, the code works fine.  snprintf here is going to return a number
between 18-32 range.  It's not going to overflow the PAGE_SIZE buffer.

It would be nicer to use scnprint() but that's an unrelated patch.

Also this is all dead code.  It needs to be converted to sysfs.  The
caller is ifdeffed out because it doesn't compile.

regards,
dan carpenter