From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753379AbbKQO3d (ORCPT ); Tue, 17 Nov 2015 09:29:33 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:57104 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751616AbbKQO3b (ORCPT ); Tue, 17 Nov 2015 09:29:31 -0500 Subject: Re: [PATCH 1/3 v7] livepatch: add old_sympos as disambiguator field to klp_func To: Jiri Kosina References: <1447431804-18786-1-git-send-email-chris.j.arges@canonical.com> <1447693391-10065-1-git-send-email-chris.j.arges@canonical.com> <1447693391-10065-2-git-send-email-chris.j.arges@canonical.com> Cc: live-patching@vger.kernel.org, jpoimboe@redhat.com, sjenning@redhat.com, Vojtech Pavlik , pmladek@suse.com, jeyu@redhat.com, linux-kernel@vger.kernel.org From: Chris J Arges Message-ID: <564B39C7.6070605@canonical.com> Date: Tue, 17 Nov 2015 08:29:27 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/16/2015 03:59 PM, Jiri Kosina wrote: > On Mon, 16 Nov 2015, Chris J Arges wrote: > >> In cases of duplicate symbols, old_sympos will be used to disambiguate >> instead of old_addr. By default old_sympos will be 0, and patching will >> only succeed if the symbol is unique. Specifying a positive value will >> ensure that occurrence of the symbol in kallsyms for the patched object >> will be used for patching if it is valid. >> >> In addition, make old_addr an internal structure field not to be specified >> by the user. Finally, remove klp_find_verify_func_addr as it can be >> replaced by klp_find_object_symbol directly. >> >> Support for symbol position disambiguation for relocations is added in the >> next patch in this series. > > Chris, > > I am sorry to repeat myself, but the changelog is quite verbose with > respect to "what is being done", but it doesn't contain any information > whatsoever with respect to "why is this an improvement over current > state", IOW why we are changing the status quo at all. > > This absolutely has to be present in the changelog. > > Thanks, > Jiri, Ok, I had put this in the cover letter which I thought was ok as well. I'll copy those parts into this commit message as well. Here is the text below. Let me know if this is sufficient. " Currently, patching objects with duplicate symbol names fail because the creation of the sysfs function directory collides with the previous attempt. Appending old_addr to the function name is problematic as it reveals the address of the function being patched to a normal user. Using the symbol's occurrence in kallsyms to postfix the function name in the sysfs directory solves the issue of having consistent unique names and ensuring that the address is not exposed to a normal user. In addition, using the symbol position as the user's method to disambiguate symbols instead of addr allows for disambiguating symbols in modules as well for both function addresses and for relocs. This also simplifies much of the code. Special handling for kASLR is no longer needed and can be removed. The klp_find_verify_func_addr function can be replaced by klp_find_object_symbol, and klp_verify_vmlinux_symbol and its callback can be removed completely. " --chris