Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752147AbbGYEwY (ORCPT ); Sat, 25 Jul 2015 00:52:24 -0400 Received: from mail-la0-f52.google.com ([209.85.215.52]:35759 "EHLO mail-la0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751001AbbGYEwW convert rfc822-to-8bit (ORCPT ); Sat, 25 Jul 2015 00:52:22 -0400 MIME-Version: 1.0 In-Reply-To: <20150724152955.GC21441@nazgul.tnic> References: <049fdbab8ae2ecac1c8b40ecd558e9df45ccd5d3.1437592883.git.luto@kernel.org> <20150724152955.GC21441@nazgul.tnic> From: Andy Lutomirski Date: Fri, 24 Jul 2015 21:52:01 -0700 Message-ID: Subject: Re: [PATCH v3 1/3] x86/ldt: Make modify_ldt synchronous To: Borislav Petkov Cc: Andy Lutomirski , Peter Zijlstra , Steven Rostedt , "security@kernel.org" , X86 ML , Sasha Levin , "linux-kernel@vger.kernel.org" , Konrad Rzeszutek Wilk , Boris Ostrovsky , Andrew Cooper , Jan Beulich , xen-devel , stable Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2427 Lines: 80 On Fri, Jul 24, 2015 at 8:29 AM, Borislav Petkov wrote: > Let it stick out: > > if (!cpumask_equal(mm_cpumask(current_mm), cpumask_of(smp_processor_id()))) > smp_call_function(flush_ldt, current_mm, 1); I see your wide terminal and raise you a complete rewrite of that function. Sigh, why did I assume the old code was the right way to do it? >> #endif >> - } >> - if (oldsize) { >> - paravirt_free_ldt(oldldt, oldsize); >> - if (oldsize * LDT_ENTRY_SIZE > PAGE_SIZE) >> - vfree(oldldt); >> - else >> - put_page(virt_to_page(oldldt)); >> - } >> - return 0; >> + preempt_enable(); >> } >> >> -static inline int copy_ldt(mm_context_t *new, mm_context_t *old) >> +static void free_ldt_struct(struct ldt_struct *ldt) >> { >> - int err = alloc_ldt(new, old->size, 0); >> - int i; >> - >> - if (err < 0) >> - return err; >> - >> - for (i = 0; i < old->size; i++) >> - write_ldt_entry(new->ldt, i, old->ldt + i * LDT_ENTRY_SIZE); >> - return 0; >> + if (unlikely(ldt)) { > > Save an indentation level: > > int alloc_size; > > if (!ldt) > return; > > alloc_size = sizeof(struct ldt_struct) + ldt->size * LDT_ENTRY_SIZE; > Hah¸ we both missed it. This is wrong. (Fix your backport!) > ... > >> + paravirt_free_ldt(ldt->entries, ldt->size); >> + if (alloc_size > PAGE_SIZE) >> + vfree(ldt->entries); >> + else >> + put_page(virt_to_page(ldt->entries)); I'm not sure this is correct, so I changed it to something obviously correct (kmalloc/kfree). >> >> - fill_ldt(&ldt, &ldt_info); >> - if (oldmode) >> - ldt.avl = 0; >> + if (old_ldt) { >> + memcpy(new_ldt->entries, old_ldt->entries, >> + oldsize * LDT_ENTRY_SIZE); >> + } > > Single if-statement doesn't need {} and you don't absolutely need to > keep 80cols. Just let it stick out. You read too many of Linus' comments about using wider terminals :) --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/