From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] sysctl: Don't overwrite array size variable when it is set on error earlier Date: Wed, 25 Mar 2015 17:12:06 +0000 Message-ID: <5512EC66.1080009@citrix.com> References: <1427303386-3513-1-git-send-email-boris.ostrovsky@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1427303386-3513-1-git-send-email-boris.ostrovsky@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Boris Ostrovsky , ian.campbell@citrix.com, ian.jackson@eu.citrix.com, jbeulich@suse.com, keir@xen.org, tim@xen.org Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 25/03/15 17:09, Boris Ostrovsky wrote: > When querying CPU topology, if caller-provided array size is smaller than > number of online CPUs then, in addition to returning -ENOBUFS, sysctl is > expected to provide back this number. However, this value, stored in 'i', > is overwritten in the subsequent loop's control statement. > > Make sure we don't do this by converting the loop to 'while'. > > Signed-off-by: Boris Ostrovsky > Reported-by: Andrew Cooper > --- > xen/common/sysctl.c | 6 +++++- > 1 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c > index a8c629f..b83d230 100644 > --- a/xen/common/sysctl.c > +++ b/xen/common/sysctl.c > @@ -338,8 +338,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) > ret = -ENOBUFS; > i = num_cpus; > } > + else > + i = 0; > > - for ( i = 0; i < num_cpus; i++ ) > + while ( i < num_cpus ) This would be fine to keep as "for ( ; i < num_cpus; i++)", and helps avoid an issue if someone introduces a continue; in the future. As for the fix itself, Reviewed-by: Andrew Cooper > { > xen_sysctl_cputopo_t cputopo; > > @@ -363,6 +365,8 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) > ret = -EFAULT; > break; > } > + > + i++; > } > } > else