Return-path: Received: from adelie.canonical.com ([91.189.90.139]:37523 "EHLO adelie.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754137Ab1BGEKN (ORCPT ); Sun, 6 Feb 2011 23:10:13 -0500 Date: Sun, 6 Feb 2011 22:10:07 -0600 From: Seth Forshee To: Wolfgang Kufner , Ivo Van Doorn Cc: Gertjan van Wingerde , "John W. Linville" , linux-wireless@vger.kernel.org, users@rt2x00.serialmonkey.com Subject: Re: Missing skb_pad() return value checks in rt2x00 driver Message-ID: <20110207041007.GA14242@thinkpad-t410> References: <20110131214515.GC6368@thinkpad-t410> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-wireless-owner@vger.kernel.org List-ID: On Sun, Feb 06, 2011 at 08:37:21PM +0100, Wolfgang Kufner wrote: > Hi Ivo, > > On Sun, Feb 6, 2011 at 5:17 PM, Ivo Van Doorn wrote: > > Between here and where you added the padding are a couple of function > > calls which use the skb->len field. So this patch would change the value what > > they expect. Have you checked the possible impact? > > I don't think skb_pad() touches skb->len. It writes zeros into the tailroom: Agreed, it doesn't look like skb_pad() affects that field. One point of concern though would be operations that changed the length so that the amount of padding applied was no longer correct. That isn't the case here, but it does make more sense logically that the padding would follow any adjustments to skb->len. My reason for moving it was that if the padding failed at the original location some data would have already been written to the hardware, potentially leaving it in a bad state (at a minimum it looks like beaconing would be left disabled). Maybe a better option would be to leave the padding at the original location and add some cleanup for the failure case. I don't have this hardware and am not familiar with it, but I'd be happy to rework the patch if someone who knows the hardware can advise me on what cleanup should be done.