From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752653AbbJTHWV (ORCPT ); Tue, 20 Oct 2015 03:22:21 -0400 Received: from mail-lf0-f50.google.com ([209.85.215.50]:33861 "EHLO mail-lf0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752245AbbJTHWR (ORCPT ); Tue, 20 Oct 2015 03:22:17 -0400 From: Rasmus Villemoes To: "Nelson\, Shannon" Cc: "Kirsher\, Jeffrey T" , "Brandeburg\, Jesse" , "Wyborny\, Carolyn" , "Skidmore\, Donald C" , "Vick\, Matthew" , "Ronciak\, John" , "Williams\, Mitch A" , "intel-wired-lan\@lists.osuosl.org" , "netdev\@vger.kernel.org" , "linux-kernel\@vger.kernel.org" Subject: Re: [PATCH] intel: i40e: fix confused code Organization: D03 References: <1445115499-28728-1-git-send-email-linux@rasmusvillemoes.dk> X-Hashcash: 1:20:151020:mitch.a.williams@intel.com::3S8SLDtjcwpTj//G:0000000000000000000000000000000000008Wl X-Hashcash: 1:20:151020:intel-wired-lan@lists.osuosl.org::TGNeBlMCCpO/l3ta:000000000000000000000000000000QRz X-Hashcash: 1:20:151020:shannon.nelson@intel.com::JF1DVp4A4NIe1Ih+:000000000000000000000000000000000000014fM X-Hashcash: 1:20:151020:carolyn.wyborny@intel.com::Z/9//Frdw7zCpCUn:00000000000000000000000000000000000015VI X-Hashcash: 1:20:151020:jesse.brandeburg@intel.com::XW37G1/YnbwrIJyQ:000000000000000000000000000000000001Fs6 X-Hashcash: 1:20:151020:netdev@vger.kernel.org::zfkslgEbqjzRtoy0:00000000000000000000000000000000000000014Hm X-Hashcash: 1:20:151020:matthew.vick@intel.com::edxfQt5gytyj+9gj:00000000000000000000000000000000000000037b6 X-Hashcash: 1:20:151020:linux-kernel@vger.kernel.org::gBsHhWGk5gT9X7AS:0000000000000000000000000000000002/1/ X-Hashcash: 1:20:151020:john.ronciak@intel.com::4klTk/D0Y1QUMlvS:0000000000000000000000000000000000000004GV1 X-Hashcash: 1:20:151020:jeffrey.t.kirsher@intel.com::0aGjqB5XgCKdsQyq:00000000000000000000000000000000004efp X-Hashcash: 1:20:151020:donald.c.skidmore@intel.com::DwOkVm7FSTPug3ey:000000000000000000000000000000000087/1 Date: Tue, 20 Oct 2015 09:22:14 +0200 In-Reply-To: (Shannon Nelson's message of "Mon, 19 Oct 2015 16:56:15 +0000") Message-ID: <87wpuim2c9.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 19 2015, "Nelson, Shannon" wrote: >> From: Rasmus Villemoes [mailto:linux@rasmusvillemoes.dk] >> Sent: Saturday, October 17, 2015 1:58 PM >> Subject: [PATCH] intel: i40e: fix confused code >> >> This code is pretty confused. The variable name 'bytes_not_copied' >> clearly indicates that the programmer knew the semantics of >> copy_{to,from}_user, but then the return value is checked for being >> negative and used as a -Exxx return value. >> >> I'm not sure this is the proper fix, but at least we get rid of the >> dead code which pretended to check for access faults. >> >> Signed-off-by: Rasmus Villemoes > > I believe this patch is unnecessary: if the value is negative, then it > already is an error code giving some potentially useful information. > When I dig into the copy_to_user() code, I see in the comments for > put_user() that -EFAULT is the error being returned. Thanks, this was precisely the kind of confusion I'm talking about: copy_{from,to}_user _never_ returns a negative value. It returns precisely what the very explicit variable name hints. This is in contrast to the single-scalar functions get_user/put_user, which do return -EFAULT for error and 0 for success. (See also lines 479-519 of Documentation/DocBook/kernel-hacking.tmpl). In the entire kernel source tree, two files contain a check for the return value from copy_{from,to}_user being negative. It will never trigger, so might as well be removed - except if it was _supposed_ to be checking for access violations, in which case one should probably replace it with actually handling it. Try git grep -C2 -E 'copy_(from|to)_user' drivers/net/ethernet/ Rasmus