IronPort-PHdr: =?us-ascii?q?9a23=3AYbzw/RGls2hHTjdCWF/6C51GYnF86YWxBRYc798d?=
 =?us-ascii?q?s5kLTJ7yp8WwAkXT6L1XgUPTWs2DsrQf2rqQ6/iocFdDyK7JiGoFfp1IWk1Nou?=
 =?us-ascii?q?QttCtkPvS4D1bmJuXhdS0wEZcKflZk+3amLRodQ56mNBXdrXKo8DEdBAj0OxZr?=
 =?us-ascii?q?KeTpAI7SiNm82/yv95HJbQhFgDmwbaluIBmqsA7cqtQYjYx+J6gr1xDHuGFIe+?=
 =?us-ascii?q?NYxWNpIVKcgRPx7dqu8ZBg7ipdpesv+9ZPXqvmcas4S6dYDCk9PGAu+MLrrxjD?=
 =?us-ascii?q?QhCR6XYaT24bjwBHAwnB7BH9Q5fxri73vfdz1SWGIcH7S60/VC+85Kl3VhDnlC?=
 =?us-ascii?q?YHNyY48G7JjMxwkLlbqw+lqxBm3oLYfJ2ZOP94c6jAf90VWHBBU95RWSJfDIOz?=
 =?us-ascii?q?bZYBAeQCM+lXs4bzqFwArQelCAmwHePvxSNEimHq0aA41ekqDAHI3BYnH9ILqH?=
 =?us-ascii?q?nYosn4NKMWUe+ryqnD0DfNb+5N1jjm9ofEfA0qrPaQULJ0dcre108vGxnHgFWN?=
 =?us-ascii?q?soPqJC2a2/8QvGeF6+pgUfijhHIgqwF0uzWiwNonhIrRho8Ny13J9j91zJg7KN?=
 =?us-ascii?q?GmUkJ3fN6pHZRKuyyeNIZ6Wt4uTmBrtSog17ELuZq2cDIUxJg6yRPTceGLfoeV?=
 =?us-ascii?q?7h/lSe2fOy13hGh/d7K6nxuy9E+gxfDiWcSsy1ZKqzZFksHLtnAQyxzf8siHRe?=
 =?us-ascii?q?V5/kemwTuAyRvT5ftLLEwuiKXUNZohwropmpoUrETDAjT5mELrjK+Qa0oo5PKk?=
 =?us-ascii?q?6+X/YrXmo5+dOJV4hR35MqQrgsC/AOI4PRYSX2WD+Omx16fv8VP3TblUlPE6j6?=
 =?us-ascii?q?nUvZ/AKckfpaO1GwpV3Zwi6xa7ATemytMYnXwfIV9ZfBKHi5bmO1fULP76EPew?=
 =?us-ascii?q?mE+jnylwyv/bILLhBpHNImLfn7fmeLZx81RcxxYrzdBD+5JUDakMIPbyWk/3qd?=
 =?us-ascii?q?zZAQY1Mw+qzOb9DtVyyIceVHmRAq+WLqzSq0WE5uExLOmWYo8apjL9J+Ii5/70?=
 =?us-ascii?q?gn9q0WMaKOPm+ZwYYXbwMelgP0Wee2LhzZ1JRWMNsQM4Q8TmhVmeWCJeajC5WK?=
 =?us-ascii?q?dqonlvDIOgEJeGQJynqLOG2yi/E5JMYX1eERaHFnK+M83QX/YKdTLXIcJ7lDEA?=
 =?us-ascii?q?faauRpVn1hy0sgL+jb19IbyH1DcfsMep/dxx6uubtQw4/zE+R5C012WASSdUg2?=
 =?us-ascii?q?kCShc60aR750d6zwHQguBDn/VEGIkLtLtyWQAgOMuZlrQiBg=3D=3D?=
From: "Allen Hubbe" <Allen.Hubbe@dell.com>
To: "'Logan Gunthorpe'" <logang@deltatee.com>,
        <linux-ntb@googlegroups.com>, <linux-kernel@vger.kernel.org>
Cc: "'Jon Mason'" <jdmason@kudzu.us>
References: <20171209000217.18366-1-logang@deltatee.com> <20171209000217.18366-2-logang@deltatee.com> <000201d37290$81605eb0$84211c10$@dell.com> <d06cf74e-125c-6281-9361-22bac3468251@deltatee.com>
In-Reply-To: <d06cf74e-125c-6281-9361-22bac3468251@deltatee.com>
Subject: RE: [PATCH 2/2] ntb_hw_switchtec: Check for alignment of the buffer in mw_set_trans()
Date: Mon, 11 Dec 2017 14:17:38 -0500
Message-ID: <000301d372b4$b6042100$220c6300$@dell.com>
MIME-Version: 1.0
Content-Type: text/plain;
        charset="utf-8"
Thread-Index: AQHTcIEAmu1N6hj0nUuwK+dCMVjXlaM+OxnQgAB+zgD//8tysA==
Content-Language: en-us
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 1205
Lines: 28

From: Logan Gunthorpe

> mw_get_align doesn't communicate the fact that the buffer has to be
> aligned by its size.

Is that not the purpose of the addr_align out parameter of ntb_mw_get_align()?

> It may also be that all hardware does not have this
> restriction (ie. if the hardware adds to the base address instead of
> just replacing the lower bits).
> 
> There is definitely a need to print this error somewhere as I hit this
> case and it caused very weird behavior. It was a huge pain to debug.
> Also, it's a security issue and huge bug if we end up mapping the memory
> we didn't think we were mapping.

Of course the driver should validate its parameters not allow bad mappings.  I was only commenting on the dev_err() message to the console.

What makes best sense for client drivers with regards to ntb api changes is a fair argument.  Let's see what others say.

> I don't think it's a good idea for us
> to require clients to check this as that requires a number of checks and
> a client author may forget to add it to their driver. I'd maybe go with
> a check in ntb_mw_set_trans before calling the driver, but that only
> makes sense if all hardware has the same requirement.
> 
> Logan